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

Very nice branch Jay.
I have some other minor suggestions, and I am well over my EOD, so I'll
QA this on Monday.
Thank you!

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

https://codereview.appspot.com/132770043/diff/60001/quickstart/platform_support.py#newcode26
quickstart/platform_support.py:26: import platform
Pre-existing, by could you please add a new line here separating stdlib
imports from quickstart ones?

The latter must also be put in alphabetical order.

from quickstart import (
     settings,
     utils,
)

https://codereview.appspot.com/132770043/diff/60001/quickstart/platform_support.py#newcode117
quickstart/platform_support.py:117: Also return a flag indicating
whetehr the user requested to customize
Typo: whetehr.

https://codereview.appspot.com/132770043/diff/60001/quickstart/platform_support.py#newcode130
quickstart/platform_support.py:130: if juju_command != "" and
juju_command != platform_command:
"if juju_command:" is sufficient here, an empty string evaluates to
False.
See PEP8: http://legacy.python.org/dev/peps/pep-0008/#id41

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

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app.py#newcode1646
quickstart/tests/test_app.py:1646: class
TestRequiresSudo(unittest.TestCase):
TestJujuRequiresSudo?

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app.py#newcode1659
quickstart/tests/test_app.py:1659:
self.assertTrue(app.juju_requires_sudo('local', version, False))
We could pass the version as second argument to assertTrue here, so
that, in case of failure, we know what version failed:

self.assertTrue(
     app.juju_requires_sudo('local', version, False),
     version)

Or similar.

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app.py#newcode1664
quickstart/tests/test_app.py:1664:
self.assertFalse(app.juju_requires_sudo('local', version, False))
Ditto.

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app.py#newcode1668
quickstart/tests/test_app.py:1668: with
self.assertRaises(app.ProgramExit):
test_app defines a ProgramExitTestsMixin, which lets you easily ensure
that the exit message is also correct, e.g.:

with self.assert_program_exit('bad wolf'):
    app.juju_requires_sudo('local', version, True)

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

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_platform_support.py#newcode178
quickstart/tests/test_platform_support.py:178: def
test_getenv_fails_when_ignore_env_var(self):
This test seems no longer valid, and we need to add a test for the case
JUJU is set to the default command (customized should be false).

Also, expected is starting to be a little be vague on those tests.
I'd read them better, for example, like:

expected_command = '/custom/juju'
with mock.patch('os.environ', {'JUJU': expected_command}):
     command, customized = platform_support.get_juju_command(None)
self.assertEquals(expected_command, command)
self.assertTrue(customized)

https://codereview.appspot.com/132770043/diff/60001/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/132770043/diff/60001/quickstart/utils.py#newcode347
quickstart/utils.py:347: Handles the special case of no patch level by
defaulting to 0.
Since the other parts of the docstring use imperative, I'd do the same
here: s/Handles/Handle/.

https://codereview.appspot.com/132770043/diff/60001/quickstart/utils.py#newcode360
quickstart/utils.py:360: try:
Uhm... What if the ValueError is due to the int conversion?
Do we need to retry the same int conversion twice?
What do you think about something like:
     parts = version_string.split('.', 2)
     if len(parts) == 2:
         parts.append(0)
     try:
         major, minor, patch = map(int, parts)
     ...

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