Code review comment for lp:~frankban/juju-quickstart/env-manage-models-meta

This branch looks good. None of my comments are blockers, more like
"things you might like to consider", so this LGTM.

https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py
File quickstart/models/fields.py (right):

https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#newcode128
quickstart/models/fields.py:128: return super(StringField,
self).normalize(value or None)
If the given value is the empty string then None will be passed to
.normalize and he default will be returned. That doesn't sound right.

https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#newcode164
quickstart/models/fields.py:164: return self.default
So it is not possible to have an empty string as a field value? I've
seen systems with that policy before and it come back to bite them in
the long run. However, it looks like we're already there so I'm not
sure it is something we should address right this minute.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py
File quickstart/tests/models/test_envs.py (right):

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py#newcode514
quickstart/tests/models/test_envs.py:514: # The local environment
metadata is correctly returned.
It would be nice to expand "correctly returned" to describe a little
more what that means.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py#newcode526
quickstart/tests/models/test_envs.py:526: # The ec2 environment metadata
is correctly returned.
Same comment for "correctly returned" as above.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py#newcode641
quickstart/tests/models/test_envs.py:641: self.assertEqual(expected,
obtained)
One of my personal preferences, for your consideration: I try to avoid
variable names in tests like "expected" and "obtained" and instead try
to communicate more about the values held. For example, above I might
call them "valid_pairs" and "mapped_pairs". That way the assertion says
more about the truth the test is trying to get at:
self.assertEqual(mapped_pairs, valid_pairs)

Note that making the asserts read this way generally means the
"obtained" value is the first argument and the "expected" argument is
second. Python's test infrastructure is fine with this, but we have to
make sure any test-specific assertion helpers are too.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py#newcode650
quickstart/tests/models/test_envs.py:650: self.assertEqual(help,
field.help)
I suspect you intended to pass field.name as the third argument to this
assert too.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py#newcode689
quickstart/tests/models/test_envs.py:689: self.assertEqual({},
envs.validate(self.env_metadata, env_data))
It took me a while to understand this test. Something like this might
have helped:

# To be valid, env_data must include the required values.
env_data = {'string-required': 'a string'}
validation_errors = envs.validate(self.env_metadata, env_data)
# No validation errors were found.
self.assertEqual(validation_errors, {})

Another thing was that the phrase "at least" primed me to expect the
test to be demonstrating invalid data instead of valid.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py#newcode741
quickstart/tests/models/test_envs.py:741: 'string-default': '\t another
one',
Changing the values by removing whitespace makes me nervous. Maybe for
the kinds of values we deal with it is OK, but I've been bitten by this
more than once. It seems better to me to let the occasional user screw
up with whitespace than to prevent another user from doing what they wan
to do (like having a multi-line config value).

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py#newcode776
quickstart/tests/models/test_envs.py:776: # Since this value is the
default one, the field is excluded.
Given that defaults can change over time, I would be cautious about
implicitly removing default values that the user had explicitly
provided. If they provided a value we should assume they want to avoid
depending on the default.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_envs.py#newcode785
quickstart/tests/models/test_envs.py:785: # reason the field is
excluded.
This is very nicely commented.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_fields.py
File quickstart/tests/models/test_fields.py (right):

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_fields.py#newcode318
quickstart/tests/models/test_fields.py:318: # If the field is not
required no errors are raised.
Since you used a comma in the comment for the next test, you probably
intended to have one here too.

https://codereview.appspot.com/41350043/