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 58065: Added initial code for the python mesoshttp package.
Date Thu, 06 Apr 2017 00:15:16 GMT

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



One quick comment from glancing through the patch. Why not add the package as mesos/http instead
of mesoshttp?
I think we should rename lib/mesos in cli_new to lib/cli. That should be one nice isolated
patch.

Also small things like adding packages to the virtualenv and the gitignore file should be
separate patches.
The rule of thumb is to group things together that function together, so you can write clear
concise commit messages about the change. Honestly, I'd even split the virtualenv changes
into two patches. One for the pytest additions explaining how they will be used in a future
patch to run unit tests for the cli, and one for the mock explaining its intended usage. It
may seem like a lot of "unnecessary" work up front, but it makes it much easier as a reviewer
to come in and make a decision right away as to whether a patch is ready to be committed or
not.
  
It also makes it easier to track changes and revert them in small isolated chunks if necessary.

- Kevin Klues


On April 6, 2017, 12:13 a.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58065/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 12:13 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added initial code for the python mesoshttp package.
> 
> 
> Diffs
> -----
> 
>   src/cli_new/bootstrap 6d62e9adf1d543ed00a3a2cf2484edf1c33ee443 
>   src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d40000318f2d0e439a6edf 
>   src/python/.gitignore PRE-CREATION 
>   src/python/lib/mesoshttp/__init__.py PRE-CREATION 
>   src/python/lib/mesoshttp/exceptions.py PRE-CREATION 
>   src/python/lib/mesoshttp/http.py PRE-CREATION 
>   src/python/lib/tests/mesoshttp/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/mesoshttp/test_http.py PRE-CREATION 
>   support/mesos-style.py 6489c2de8cc8e14f28ef89c7604c94d3eaaaadff 
> 
> 
> Diff: https://reviews.apache.org/r/58065/diff/1/
> 
> 
> Testing
> -------
> 
> unit tests:
> $ source src/cli_new/activate
> $ pytest --cov src/python/lib/mesoshttp --cov-report term-missing src/python/lib
> 
> 
> Thanks,
> 
> Eric Chung
> 
>


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