Code review comment for lp:~frankban/juju-quickstart/env-manage-fields
Revision history for this message
| Gary Poster (gary) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
LGTM and nice work, with consideration of various fairly trivial
comments.
I'm interested in bac's comments. Calling something Zope-like these
days is unfortunately not a complement. Perhaps he's referring to using
classes for fields? Certainly these fields could be data as well: your
classes really simply have some flags and some functions, and the
validation functions could be composed (more?) attractively
functionally, to my eyes. That said, while I am intrigued by the idea
of simplifying the fields into data, and think it might be a win, I
don't think it will be a big enough win to justify refactoring at this
time; and I think people with an OOP bent might not like the refactoring
at all.
Moreover, maybe that's not anywhere close to what bac means?
https:/
File quickstart/
https:/
quickstart/
sequence of fields. Those fields describe
...associated with a sequence of fields.
https:/
quickstart/
string field that can be
It strikes me that this functionality is along a different logical axis
than field type. I'd be tempted to have the code look for a generate
method, for instance, as a flag: if it exists, it can autogenerate (a
string, a number, whatever). I don't think this is very important: just
a suggestion.
https:/
quickstart/
widget.
Which effectively means multi-line string, right? Might as well be
explicit about it.
https:/
quickstart/
piece of information
Is this...
the key identifying a specific piece of information in the environments
data
...or not? Might be good to clarify, either way.
https:/
quickstart/
required=True, editable=True):
I generally prefer flags to default to false, but I think this is
probably the right compromise between clarity and convenience for
required and editable...unless we can choose different names?
optional=False, readonly=False
readonly conveys the idea well. Less sure about "optional"
https:/
quickstart/
identifier is required."""
Run-on sentence. Any of the following are better
Initialize a field. Only...
Initialize a field; only...
I prefer the period in this case.
https:/
quickstart/
When would this happen? Or is this just insurance?
https:/
quickstart/
Maybe "return self.normalize(
https:/
quickstart/
Maybe call out that this has no field_type because it is the default?
https:/
quickstart/
You could delete this line if you make my suggested change to the base
class' validate method.
https:/
quickstart/
string or None: in this case the
I suggest the following.
Return None if the value is an empty string or None. In these cases,
the field value is considered not set.
https:/
quickstart/
The duplication of this code from the base class is one possible
argument for the greater flexibility of using functional composition to
build validation. Not suggesting the change, because that would slow
things down, but I'm comparing this in my mind with an approach in the
"Functional Javascript" book I've been reading.
https:/
quickstart/
use if the value is unset (None).
Seems like the default functionality could be shared in the base class.
If you wanted to be paranoid, you could even run it through the
"validate" function.
https:/
quickstart/
AutoGeneratedSt
https:/
quickstart/
tuple(choices) just to be paranoid?
https:/
File quickstart/
https:/
quickstart/
self.assertEqua
I guess it is good that all fields can handle None, but guaranteeing
that None is displayed as u'None' doesn't seem ideal on the face of it.
https:/