mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.
Date Fri, 29 Sep 2017 15:48:18 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review186685
-----------------------------------------------------------




src/python/lib/mesos/exceptions.py
Lines 21 (patched)
<https://reviews.apache.org/r/61172/#comment263477>

    We don't need this because we don't import anything.



src/python/lib/mesos/exceptions.py
Lines 50 (patched)
<https://reviews.apache.org/r/61172/#comment263479>

    Please reword as "Could not fetch..." All exceptions will be wrapped with an "Error" when
caught at a higher level.



src/python/lib/mesos/exceptions.py
Lines 69 (patched)
<https://reviews.apache.org/r/61172/#comment263481>

    Ditto



src/python/lib/mesos/http.py
Lines 33 (patched)
<https://reviews.apache.org/r/61172/#comment263484>

    Typically we separate these out one per line for consistency



src/python/lib/mesos/http.py
Lines 249 (patched)
<https://reviews.apache.org/r/61172/#comment263491>

    What is mesos specific about this class to justify calling it `MesosResource`?



src/python/lib/tests/conftest.py
Lines 27 (patched)
<https://reviews.apache.org/r/61172/#comment263485>

    Until now we haven't really been using python decorators anywhere. It seems weird to introduce
them here without having them anywhere else. Independently, why not run this test using the
other test infrastructure we have for actually spinning up a master / agent?



src/python/lib/tests/test_exceptions.py
Lines 27 (patched)
<https://reviews.apache.org/r/61172/#comment263487>

    Again, it seems weird to introduce this without introducing it everywhere.



src/python/lib/tests/test_http.py
Lines 41 (patched)
<https://reviews.apache.org/r/61172/#comment263488>

    Ditto



src/python/lib/tests/test_http.py
Lines 124 (patched)
<https://reviews.apache.org/r/61172/#comment263490>

    In general, all of these tests should have the same look and feel as the existing tests.

    
    If we think the way you have organized these is the right way to go, we can change the
exisiting ones to follow this pattern, but we should strive for consistency in whatever we
finally commit back.
    
    As of now, it's hard for me to glance through these patches and knwo whether everything
is correct or not because I am not familiar with the style you are going for - it's different
than the style used throughout the rest of the code base.


- Kevin Klues


On Sept. 21, 2017, 4:27 a.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Sept. 21, 2017, 4:27 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.
> 
> 
> 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/5/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>


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