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

*** 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))?

Done.

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?

Done.

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'))

Done.

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.

Done.

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)

Done.

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