Code review comment for lp:~frankban/juju-quickstart/env-manage-fields
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!)
*** Submitted:
Env management: fields.
See the module docstring for an
explanation of how those fields
will be used.
Tests: `make check`.
No QA required.
R=bac, gary.poster
CC=
https:/
https:/
File quickstart/
https:/
quickstart/
sequence of fields. Those fields describe
On 2013/12/11 14:15:21, gary.poster wrote:
> ...associated with a sequence of fields.
Done.
https:/
quickstart/
string field that can be
On 2013/12/11 14:15:21, gary.poster wrote:
> 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.
Good idea, done.
https:/
quickstart/
widget.
On 2013/12/11 14:15:21, gary.poster wrote:
> Which effectively means multi-line string, right? Might as well be
explicit
> about it.
Right, fixed.
https:/
quickstart/
piece of information
On 2013/12/11 14:15:21, gary.poster wrote:
> Is this...
> the key identifying a specific piece of information in the
environments data
> ...or not? Might be good to clarify, either way.
Done.
https:/
quickstart/
must be considered immutable.
On 2013/12/11 13:41:16, bac wrote:
> This definition seems backwards. s/immutable/
Done.
https:/
quickstart/
On 2013/12/11 13:41:16, bac wrote:
> Should there be a trap for that situation? A _validated flag?
Not sure. We don't have state in those fields, e.g.:
field = MyField('foo')
field.validate(42)
field.normalize
https:/
quickstart/
required=True, editable=True):
On 2013/12/11 14:15:21, gary.poster wrote:
> 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"
As agreedd, we now use required=False, readonly=False.
https:/
quickstart/
identifier is required."""
On 2013/12/11 14:15:21, gary.poster wrote:
> Run-on sentence. Any of the following are better
> Initialize a field. Only...
> Initialize a field; only...
> I prefer the period in this case.
Done.
https:/
quickstart/
On 2013/12/11 14:15:21, gary.poster wrote:
> When would this happen? Or is this just insurance?
Just insurance, but good catch. We can assume we always work with
unicode strings, deleted.
https:/
quickstart/
On 2013/12/11 14:15:21, gary.poster wrote:
> Maybe "return self.normalize(
As discussed, changed the validate contract so that either a ValueError
is raised or None is returned.
https:/
quickstart/
On 2013/12/11 14:15:21, gary.poster wrote:
> Maybe call out that this has no field_type because it is the default?
Added this comment in the base Field.
https:/
quickstart/
string or None: in this case the
On 2013/12/11 14:15:21, gary.poster wrote:
> 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.
Done.
https:/
quickstart/
use if the value is unset (None).
On 2013/12/11 14:15:21, gary.poster wrote:
> 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.
Validation done. Since it seems the default is only required here, I'll
leave its handling in this subclass for now.
https:/
quickstart/
On 2013/12/11 14:15:21, gary.poster wrote:
> AutoGeneratedSt
Done.
https:/
quickstart/
On 2013/12/11 14:15:21, gary.poster wrote:
> tuple(choices) just to be paranoid?
Done.
https:/