----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61172/#review194862 -----------------------------------------------------------
Some small issues to fix, there are also some typos in the commit description. src/python/cli_new/bootstrap Lines 72 (patched) <https://reviews.apache.org/r/61172/#comment273927> This file does not exist, is that normal? src/python/lib/mesos/exceptions.py Lines 26 (patched) <https://reviews.apache.org/r/61172/#comment273928> s/mesos/Mesos src/python/lib/mesos/exceptions.py Lines 30 (patched) <https://reviews.apache.org/r/61172/#comment273929> We use the syntax `"{msg}: {exception}".format(msg=message, exception=original_exception)` in the rest of our Python codebase. I do not have an opinion regarding the syntax but we should follow the current one in that review request and do a refactoring later if necessary. src/python/lib/mesos/exceptions.py Lines 60 (patched) <https://reviews.apache.org/r/61172/#comment273930> s/'/" src/python/lib/mesos/exceptions.py Lines 75 (patched) <https://reviews.apache.org/r/61172/#comment273931> src/python/lib/mesos/exceptions.py Lines 83 (patched) <https://reviews.apache.org/r/61172/#comment273934> Unecessary comma. src/python/lib/mesos/exceptions.py Lines 106 (patched) <https://reviews.apache.org/r/61172/#comment273935> "Given when " is unecessary. src/python/lib/mesos/http.py Lines 20 (patched) <https://reviews.apache.org/r/61172/#comment273936> s/`restful API`/`RESTful API.` src/python/lib/mesos/http.py Lines 25 (patched) <https://reviews.apache.org/r/61172/#comment273939> Let's do `from urlparse import urlparse` instead as we only use this once for that function in that file. src/python/lib/mesos/http.py Lines 33 (patched) <https://reviews.apache.org/r/61172/#comment273937> Not in alphabetical order but it makes sense due to the inheritance of these classes. src/python/lib/mesos/http.py Lines 72 (patched) <https://reviews.apache.org/r/61172/#comment273938> We should do that everywhere in the CLI, this is a useful addition. We could also use [mypy](http://www.mypy-lang.org/). src/python/lib/mesos/http.py Lines 117 (patched) <https://reviews.apache.org/r/61172/#comment273944> Will just be `urlparse(url)` following previous comment. src/python/lib/mesos/http.py Lines 245 (patched) <https://reviews.apache.org/r/61172/#comment273940> ``` :param use_gzip_encoding: boolean indicating whether to pass gzip encoding in the request headers or not ``` src/python/lib/mesos/http.py Lines 261 (patched) <https://reviews.apache.org/r/61172/#comment273941> I have some trouble reading this comment. I would write something like "We retry only when it makes sense: either due to a network partition (e.g. connection errors) or if the request failed due to a server error such as 500s, timeouts, and so on." src/python/lib/tests/test_http.py Lines 239 (patched) <https://reviews.apache.org/r/61172/#comment273943> s/resrouce/resource - Armand Grillet On Jan. 1, 2018, 1:52 a.m., Eric Chung wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61172/ > ----------------------------------------------------------- > > (Updated Jan. 1, 2018, 1:52 a.m.) > > > Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues. > > > Repository: mesos > > > Description > ------- > > Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions > modules, which provides a Resource class and its descendants for > abstracting away common operations over http connectioins with JSON > serialization. > > updated patch according to klueska's recommendations > > > addressed comments > > > Diffs > ----- > > src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 > src/python/lib/mesos/exceptions.py PRE-CREATION > src/python/lib/mesos/http.py PRE-CREATION > src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > src/python/lib/tests/conftest.py PRE-CREATION > src/python/lib/tests/test_exceptions.py PRE-CREATION > src/python/lib/tests/test_http.py PRE-CREATION > src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 > support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb > > > Diff: https://reviews.apache.org/r/61172/diff/8/ > > > Testing > ------- > > install tox > cd src/python/lib > tox > > > Thanks, > > Eric Chung > >
