Code review comment for lp://qastaging/~bac/juju-quickstart/donde-brew

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

This branch is nice, thank you!
LGTM with some changes.

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py#newcode332
quickstart/manage.py:332: """Validate the platform.
"""Validate the platform.

Exit with an error if platform is not supported by quickstart
or is missing files.
"""

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py#newcode338
quickstart/manage.py:338: return
parser.error(err.message.encode('utf-8'))
err.message should already be a byte string.
Perhaps return parser.error(bytes(err))?

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py
File quickstart/platform_support.py (right):

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode30
quickstart/platform_support.py:30: def validate_platform(pf):
s/pf/platform/? <shrug>

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode31
quickstart/platform_support.py:31: """Validate the platform has the
required files installed.
Validate the platform is supported and has the required files?

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode46
quickstart/platform_support.py:46: raise ValueError(bytes(error_msg))
An explicit conversion seems slightly better here:
raise ValueError(error_msg.encode('utf-8'))

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode49
quickstart/platform_support.py:49: b'juju-quickstart requires brew to be
installed on OS X.'
Missing trailing space to separate the two sentences.

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

https://codereview.appspot.com/108930045/diff/20001/quickstart/tests/test_platform_support.py#newcode128
quickstart/tests/test_platform_support.py:128: class
TestValidatePlatform(unittest.TestCase):
Nice tests, thank you.
This TestCase could take advantage of helpers.ValueErrorTestsMixin, so
that you can avoid some boilerplate, e.g.:

with self.assert_value_error(expected_error):
    platform_support.validate_platform(settings.OSX)

https://codereview.appspot.com/108930045/

« Back to merge proposal