Code review comment for lp:~frankban/juju-quickstart/env-manage-models-meta
Revision history for this message
| Francesco Banconi (frankban) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
Please take a look.
https:/
File quickstart/
https:/
quickstart/
self).normalize
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:/
quickstart/
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.
https:/
File quickstart/
https:/
quickstart/
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:/
quickstart/
is correctly returned.
On 2013/12/13 17:15:59, benji wrote:
> Same comment for "correctly returned" as above.
Done.
https:/
quickstart/
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.assertEqua
> 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:/
quickstart/
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:/
quickstart/
envs.validate(
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(
> # No validation errors were found.
> self.assertEqua
> Another thing was that the phrase "at least" primed me to expect the
> test to be demonstrating invalid data instead of valid.
Done.
https:/
quickstart/
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.
https:/
quickstart/
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.
https:/
File quickstart/
https:/
quickstart/
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:/