-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review181756
-----------------------------------------------------------
In _testing done_, there seems to be a missing first part asking to run the updated bootstrap
script and `source activate` under `src/python/cli_new` before running `tox`. At least these
were necessary steps for me to have `tox` running correctly.
src/python/lib/mesos/exceptions.py
Lines 26 (patched)
<https://reviews.apache.org/r/61172/#comment257401>
Quotemarks and docstring can fit in one line, a period should be added at the end, and
a blank line should be added after this line.
src/python/lib/mesos/exceptions.py
Lines 43 (patched)
<https://reviews.apache.org/r/61172/#comment257402>
For consistency let's break the line after the quotemarks and add a period after `response`.
src/python/lib/mesos/exceptions.py
Lines 51 (patched)
<https://reviews.apache.org/r/61172/#comment257403>
The brackets around `url` do not seem necessary.
src/python/lib/mesos/exceptions.py
Lines 67 (patched)
<https://reviews.apache.org/r/61172/#comment257404>
The first line of the docstring can be replaced by `A wrapper around Response objects
for HTTP Unprocessable Entity error (422).`, which fits in one line and can end with a period.
A new line can then be added explaining that it is likely due to an unrpocessable JSON entity.
src/python/lib/mesos/exceptions.py
Lines 71 (patched)
<https://reviews.apache.org/r/61172/#comment257405>
The use of `{0}` and so on is unconsistent with the previous use of `format` in this file
(which is more common in our Python codebase). The brackets around the URL seem uneccesary.
src/python/lib/mesos/http.py
Lines 33 (patched)
<https://reviews.apache.org/r/61172/#comment257407>
We generally prefer to do one import per line but it would be quite heavy in that situation.
src/python/lib/mesos/http.py
Lines 52 (patched)
<https://reviews.apache.org/r/61172/#comment257406>
s/`Does`/`Do`. This line could be rephrased to `Do a join by rstrip'ing / from base_url
and lstrp'ing / from other_url.` to be a PEP257 compliant one liner.
src/python/lib/mesos/http.py
Lines 65 (patched)
<https://reviews.apache.org/r/61172/#comment257408>
Can start with a one liner `Encapsulate the context for an HTTP resource`.
src/python/lib/mesos/http.py
Lines 101 (patched)
<https://reviews.apache.org/r/61172/#comment257409>
Can be a one liner: `Return a new Resource object at a subpath of the current resource's
URL.`.
src/python/lib/mesos/http.py
Lines 119 (patched)
<https://reviews.apache.org/r/61172/#comment257410>
Missing period, s/`verb`/`token` according to https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html.
src/python/lib/mesos/http.py
Lines 162 (patched)
<https://reviews.apache.org/r/61172/#comment257411>
Missing period.
src/python/lib/mesos/http.py
Lines 168 (patched)
<https://reviews.apache.org/r/61172/#comment257412>
Missing period.
src/python/lib/mesos/http.py
Lines 195 (patched)
<https://reviews.apache.org/r/61172/#comment257413>
To be more consistent with our Python codebase:
```
raise HTTPException('Could not load JSON from "{data}": {error}'
.format(data=resp.text, error=str(exception)))
```
src/python/lib/mesos/http.py
Lines 206 (patched)
<https://reviews.apache.org/r/61172/#comment257414>
s/`Sends`/`Send`.
src/python/lib/mesos/http.py
Lines 226 (patched)
<https://reviews.apache.org/r/61172/#comment257415>
s/`Sends`/`Send`.
src/python/lib/mesos/http.py
Lines 249 (patched)
<https://reviews.apache.org/r/61172/#comment257416>
The line 247 already states the beginning of the docstring, `Class adding the context
necessary to talk to Mesos master and agent APIs.` can be a one liner replacing it.
src/python/lib/mesos/http.py
Lines 285 (patched)
<https://reviews.apache.org/r/61172/#comment257417>
s/`verb`/`token`.
src/python/lib/mesos/http.py
Lines 331 (patched)
<https://reviews.apache.org/r/61172/#comment257418>
Missing period.
src/python/lib/mesos/http.py
Lines 371 (patched)
<https://reviews.apache.org/r/61172/#comment257419>
The brackets seem unecessary.
src/python/lib/mesos/http.py
Lines 379 (patched)
<https://reviews.apache.org/r/61172/#comment257420>
The brackets seem unecessary.
src/python/lib/mesos/http.py
Lines 383 (patched)
<https://reviews.apache.org/r/61172/#comment257421>
s/`{}`/`{error}` or `{e}` with updated `format`.
src/python/lib/requirements.in
Lines 1 (patched)
<https://reviews.apache.org/r/61172/#comment257400>
Not sure about using `<=` here and in the lines below, would use `<` so that we
keep the same main dependency versions.
src/python/lib/tests/conftest.py
Lines 30 (patched)
<https://reviews.apache.org/r/61172/#comment257422>
Missing period.
src/python/lib/tests/test_http.py
Lines 89 (patched)
<https://reviews.apache.org/r/61172/#comment257423>
This list is not very readable but I am not sure about how to improve it.
src/python/lib/tests/test_http.py
Lines 159 (patched)
<https://reviews.apache.org/r/61172/#comment257424>
Could be a one liner, is the comma necessary?
- Armand Grillet
On July 27, 2017, 7:09 a.m., Eric Chung wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 7:09 a.m.)
>
>
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added mesos.http and mesos.exceptions for CLI.
>
> 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.
>
>
> Diffs
> -----
>
> src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442
> src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818
> 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
>
>
> Diff: https://reviews.apache.org/r/61172/diff/1/
>
>
> Testing
> -------
>
> under src/python/lib, call `tox` for running unit tests. The test should pass and test
coverage should be at 100%.
>
>
> Thanks,
>
> Eric Chung
>
>
|