*** Submitted:
Fail early if brew is not installed.
Did some refactoring to list required files for each supported platform. If required files are not found, fail early with an appropriate parser error message.
R=frankban CC= https://codereview.appspot.com/108930045
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. On 2014/06/13 14:16:27, frankban wrote: > """Validate the platform.
> Exit with an error if platform is not supported by quickstart > or is missing files. > """
Done.
https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py#newcode338 quickstart/manage.py:338: return parser.error(err.message.encode('utf-8')) On 2014/06/13 14:16:27, frankban wrote: > 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): platform clashes with the module name. I started to do platform_ but I found it annoying.
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. On 2014/06/13 14:16:27, frankban wrote: > 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)) On 2014/06/13 14:16:28, frankban wrote: > 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.' On 2014/06/13 14:16:27, frankban wrote: > 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): On 2014/06/13 14:16:28, frankban wrote: > 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
*** Submitted:
Fail early if brew is not installed.
Did some refactoring to list required files for each supported platform.
If required files are not found, fail early with an appropriate parser
error message.
R=frankban /codereview. appspot. com/108930045
CC=
https:/
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/
On 2014/06/13 14:16:27, frankban wrote:
> """Validate the platform.
> Exit with an error if platform is not supported by quickstart
> or is missing files.
> """
Done.
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.
On 2014/06/13 14:16:27, frankban wrote:
> err.message should already be a byte string.
> Perhaps return parser.
Done.
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/
platform clashes with the module name. I started to do platform_ but I
found it annoying.
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.
On 2014/06/13 14:16:27, frankban wrote:
> Validate the platform is supported and has the required files?
Done.
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/
On 2014/06/13 14:16:28, frankban wrote:
> An explicit conversion seems slightly better here:
> raise ValueError(
Done.
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.'
On 2014/06/13 14:16:27, frankban wrote:
> Missing trailing space to separate the two sentences.
Done.
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
On 2014/06/13 14:16:28, frankban wrote:
> 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_
Done.
https:/ /codereview. appspot. com/108930045/