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