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 Fri, 05 Jan 2018 15:23:55 GMT

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


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