Code review comment for lp:~frankban/juju-quickstart/add-tox

Revision history for this message
Francesco Banconi (frankban) wrote :

Please take a look.

https://codereview.appspot.com/189580044/diff/1/HACKING.rst
File HACKING.rst (right):

https://codereview.appspot.com/189580044/diff/1/HACKING.rst#newcode356
HACKING.rst:356: parsing ``tox.ini``, dependencies must be listed with
the following rules:
On 2015/01/22 16:15:27, bac wrote:
> Could you try to re-word that sentence? I'm not sure what you're
saying.

Done.

https://codereview.appspot.com/189580044/diff/1/HACKING.rst#newcode358
HACKING.rst:358: - each scenario must at least include a
"{[testenv]deps}" line in its deps:
On 2015/01/22 16:15:27, bac wrote:
> Change the : to ; or ,

Done.

https://codereview.appspot.com/189580044/diff/1/MANIFEST.in
File MANIFEST.in (right):

https://codereview.appspot.com/189580044/diff/1/MANIFEST.in#newcode24
MANIFEST.in:24: # tox.ini file, removing it from this list will break
the source distribution
On 2015/01/22 16:15:27, bac wrote:
> Again, this could be worded better.

Done.

https://codereview.appspot.com/189580044/diff/1/setup.py
File setup.py (right):

https://codereview.appspot.com/189580044/diff/1/setup.py#newcode43
setup.py:43: """Return Juju Quickstart package data by walking through
the project."""
On 2015/01/22 16:15:27, bac wrote:
> A better explanation of what "package data" is would be nice.

Done.

https://codereview.appspot.com/189580044/diff/1/setup.py#newcode74
setup.py:74: for requirement in deps:
On 2015/01/22 16:15:27, bac wrote:
> Maybe add

> requirement = requirement.strip()

Done.

https://codereview.appspot.com/189580044/diff/1/tox.ini
File tox.ini (right):

https://codereview.appspot.com/189580044/diff/1/tox.ini#newcode42
tox.ini:42: basepython = python2.7
On 2015/01/22 16:15:27, bac wrote:
> Is it required to repeat here?

No it's not. Good catch!

https://codereview.appspot.com/189580044/

« Back to merge proposal