mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Armand Grillet <agril...@mesosphere.io>
Subject Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.
Date Sat, 29 Jul 2017 15:12:04 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message