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
This branch is nice, thank you!
LGTM with some changes.
https:/ /codereview. appspot. com/108930045/ diff/20001/ quickstart/ manage. py manage. py (right):
File quickstart/
https:/ /codereview. appspot. com/108930045/ diff/20001/ quickstart/ manage. py#newcode332 manage. py:332: """Validate the platform.
quickstart/
"""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 manage. py:338: return error(err. message. encode( 'utf-8' )) error(bytes( err))?
quickstart/
parser.
err.message should already be a byte string.
Perhaps return parser.
https:/ /codereview. appspot. com/108930045/ diff/20001/ quickstart/ platform_ support. py platform_ support. py (right):
File quickstart/
https:/ /codereview. appspot. com/108930045/ diff/20001/ quickstart/ platform_ support. py#newcode30 platform_ support. py:30: def validate_ platform( pf):
quickstart/
s/pf/platform/? <shrug>
https:/ /codereview. appspot. com/108930045/ diff/20001/ quickstart/ platform_ support. py#newcode31 platform_ support. py:31: """Validate the platform has the
quickstart/
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 platform_ support. py:46: raise ValueError( bytes(error_ msg)) error_msg. encode( 'utf-8' ))
quickstart/
An explicit conversion seems slightly better here:
raise ValueError(
https:/ /codereview. appspot. com/108930045/ diff/20001/ quickstart/ platform_ support. py#newcode49 platform_ support. py:49: b'juju-quickstart requires brew to be
quickstart/
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 tests/test_ platform_ support. py (right):
File quickstart/
https:/ /codereview. appspot. com/108930045/ diff/20001/ quickstart/ tests/test_ platform_ support. py#newcode128 tests/test_ platform_ support. py:128: class tform(unittest. TestCase) : ValueErrorTests Mixin, so
quickstart/
TestValidatePla
Nice tests, thank you.
This TestCase could take advantage of helpers.
that you can avoid some boilerplate, e.g.:
with self.assert_ value_error( expected_ error): support. validate_ platform( settings. OSX)
platform_
https:/ /codereview. appspot. com/108930045/