Code review comment for lp:~evarlast/juju-quickstart/which-juju

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

LGTM with a couple of minor changes.
QA ok.
Thank you!

https://codereview.appspot.com/132770043/diff/80001/quickstart/platform_support.py
File quickstart/platform_support.py (right):

https://codereview.appspot.com/132770043/diff/80001/quickstart/platform_support.py#newcode30
quickstart/platform_support.py:30: utils
Missing comma at the end of the line (utils,).

https://codereview.appspot.com/132770043/diff/80001/quickstart/platform_support.py#newcode134
quickstart/platform_support.py:134: logging.warn("a customized juju is
being used.")
no need for the final . here

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_platform_support.py
File quickstart/tests/test_platform_support.py (right):

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_platform_support.py#newcode170
quickstart/tests/test_platform_support.py:170: class
TestGetJujuCommand(unittest.TestCase):
What do you think about changing from expected/actual to
expected_command/assertTrue also in the first and third tests here?
I read the second one way better.

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_platform_support.py#newcode178
quickstart/tests/test_platform_support.py:178: def
test_getenv_fails_when_ignore_env_var(self):
The test name here seems to no longer describe the goal.

https://codereview.appspot.com/132770043/

« Back to merge proposal