*** 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://codereview.appspot.com/37820046 https://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py File quickstart/models/fields.py (right): https://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode24 quickstart/models/fields.py:24: each provider type is associated to a sequence of fields. Those fields describe On 2013/12/11 14:15:21, gary.poster wrote: > ...associated with a sequence of fields. Done. https://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode49 quickstart/models/fields.py:49: - autogenerated: indicates this is a 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode53 quickstart/models/fields.py:53: edited using the usual input edit 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode56 quickstart/models/fields.py:56: - name: the name identifying a specific 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode63 quickstart/models/fields.py:63: - editable: True if the associated value must be considered immutable. On 2013/12/11 13:41:16, bac wrote: > This definition seems backwards. s/immutable/mutable/ ? Done. https://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode77 quickstart/models/fields.py:77: been previously validated. 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(None) https://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode83 quickstart/models/fields.py:83: self, name, label=None, help='', 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode84 quickstart/models/fields.py:84: """Initialize a field, only the name 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode100 quickstart/models/fields.py:100: if isinstance(value, str): 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode117 quickstart/models/fields.py:117: return value On 2013/12/11 14:15:21, gary.poster wrote: > Maybe "return self.normalize(value) so you can show the intended use? As discussed, changed the validate contract so that either a ValueError is raised or None is returned. https://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode122 quickstart/models/fields.py:122: 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode156 quickstart/models/fields.py:156: Return None if the value is an empty 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode218 quickstart/models/fields.py:218: the default value (True or False) to 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://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode238 quickstart/models/fields.py:238: class AutoGeneratedField(StringField): On 2013/12/11 14:15:21, gary.poster wrote: > AutoGeneratedStringField? Done. https://codereview.appspot.com/37820046/diff/1/quickstart/models/fields.py#newcode257 quickstart/models/fields.py:257: self.choices = choices On 2013/12/11 14:15:21, gary.poster wrote: > tuple(choices) just to be paranoid? Done. https://codereview.appspot.com/37820046/