On 2013/12/16 12:12:04, frankban wrote: > Please take a look. Thanks for the good explanations and changes. Everything looks great. 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) > On 2013/12/13 17:15:59, benji wrote: > > 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. > Changed the module so that default is never returned by normalize: see below. https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#newcode164 > quickstart/models/fields.py:164: return self.default > On 2013/12/13 17:15:59, benji wrote: > > 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. > An IntField is always normalized to a number or None (unset). An empty string is > accepted as an input value, but what we really want is a number or a "not set" > value. Ah, gotcha. 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. > On 2013/12/13 17:15:59, benji wrote: > > It would be nice to expand "correctly returned" to describe a little more what > > that means. > Done. 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. > On 2013/12/13 17:15:59, benji wrote: > > Same comment for "correctly returned" as above. > Done. 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) > On 2013/12/13 17:15:59, benji wrote: > > 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. > Very interesting. +1 on more meaningful assertions. 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) > On 2013/12/13 17:15:59, benji wrote: > > I suspect you intended to pass field.name as the third argument to this assert > > too. > Good catch, thanks. 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)) > On 2013/12/13 17:15:59, benji wrote: > > 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. > Done. 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', > On 2013/12/13 17:15:59, benji wrote: > > 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). > Not sure about leaving leading/trailing spaces, but we should definitely allow > multi-lines values. We already do that, but I added a test to confirm the right > behavior. Ah, I misunderstood the whitespace cleaning, thinking it was more aggressive than it is. 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. > On 2013/12/13 17:15:59, benji wrote: > > 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. > This is really a great suggestion Benji, thank you! You are right, we want > explictly set values to be there when the environments.yaml is saved. I changed > the fields code so that the default value is used in the validation process but > not in the normalization one. Sounds good. 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. > On 2013/12/13 17:15:59, benji wrote: > > Since you used a comma in the comment for the next test, you probably intended > > to have one here too. > Done. https://codereview.appspot.com/41350043/