> Thanks Raphers. This is needs-fixing because there's a few coding style > violations and a test antipattern. Thanks for the review. This is a first batch of fixes. Now I'm working on improving the test file. > > s/correspond/corresponds/ > s/to store/the storing of/ Done. > > 44 + For instance a DictCharField created with the following list: > 45 + [ [...] > 50 + {'field1': 'subvalue1', 'field2': 'subvalue2'} > > I think this example needs expanding to show how assignment works. It wasn't > clear to me how you get from the first bit to the second bit, which suddenly > has values. Dood point, done > 55 + self.field_dict = OrderedDict(field_items) > > Why ordered? Shouldn't that be an option? No, I need self.fields and self.names to be ordered in the same way. But note that this is an implementation detail. > 60 + if skip_check: > 61 + self.field_dict[SKIP_CHECK_NAME] = forms.BooleanField( > 62 + required=False) > > I think SKIP_CHECK_NAME needs to be more obscure; it's quite conceivable that > someone will have a field named skip_check. Perhaps '__skip_check' ? You've got a point, but since SKIP_CHECK_NAME is exposed to the user, I think it should be readable. Instead I've added a check to make sure that no user-defined subfield is named 'skip_check'. I've added a test for that (test_DictCharField_does_not_allow_subfield_named_skip_check). > 76 + def compress(self, data): > 77 + if data: > > Docstring missing. Done. > s/splitted/split/ Fixed. > The code has 'clean_sub_fields' ... Right… fixed. > 114 + value = value if not self.skip_check else value[:-1] > > How about: > > if self.skip_check: > value = value[:-1] > > which is easier to read. We tend to eschew this logic shortcut constraint, > it's hideously ugly and error prone. Done. I've also added a comment to explain why this is for. > 111 + def clean_global(self, value): > 112 + # Remove the value corresponding to the SKIP_CHECK_NAME boolean > field > 113 + # if required. > > Missing docstring. Fixed. > 115 + if not value or isinstance(value, (list, tuple)): > 116 + is_empty = ( > 117 + not value or > 118 + not [v for v in value if v not in validators.EMPTY_VALUES]) > > *blink* > > Firstly, our coding standards say to check explicitly for None or False, and > not use "if not value". > > Second, this really needs splitting up into something that's more > understandable. I don't have any good suggestions because I can't tell what > you're trying to do here. The code appears to be doing more than the comment > suggests. Done, and I've added a comment. > 127 + raise ValidationError(self.error_messages['invalid']) > > Is this "invalid" text something that Django needs or can we write a better > error text here? This is precisely done so that you can override all the default error messages on a per-field basis: e.g.: name = forms.CharField(error_messages={'invalid': "Custom invalid message here"}) > > 129 + def clean_sub_fields(self, value): > 130 + clean_data = [] > > Missing docstring. Fixed. > 134 + fields = self.fields if not self.skip_check else self.fields[:-1] > > fields = self.fields > if self.skip_check: > fields = self.fields[:-1] > > 135 + for i, field in enumerate(fields): > > Our coding standards say to use useful variable names. Perhaps "index" here. rarg, again, this was taken from Django's code… 'index' it is… > 163 +def get_all_prefixed_values(data, name): > 164 + result = {} > > Missing docstring. Done. I've also added a test for this method. > 185 + - DictCharWidget has the (optional) ability to skip all the > validation > 186 + are instead fetch all the values prefixed by 'main_widget_' in the > 187 + input data. > > s/are instead/and instead/ ? Fixed. > > 194 + - 'value_from_datadict' which fetches the value of the data to be > 195 + processed by this form give a 'data' dictionary. We need to > > s/give/to give/ > > 211 + def render(self, name, value, attrs=None): > 212 + # value is a list of values, each corresponding to a widget > 213 + # in self.widgets. > > Docstring missing. Also the same problem with variable naming and the > conditional shortcut as above. Fixed. > 218 + id_ = final_attrs.get('id', None) > > Why the _ here? Is "id" a Django convention you want to avoid or something > else is clashing? Well, again, this was taken from Django's code and yes, I think the intent is to avoid clashing with Python's built-in 'id' (http://docs.python.org/library/functions.html#id). > > 238 + def id_for_label(self, id_): > 239 + # See the comment for RadioSelect.id_for_label() > > Missing docstring. Fixed. > 244 + def value_from_datadict(self, data, files, name): > 245 + """Extract the values for each widget from a data dict > (QueryDict).""" > > Kudos for the docstring, but it doesn't document the parameters or the return > value :) Note that all this methods without a docstring are methods from forms.widgets.MultiWidget that I had to override. But anyway, I'm adding docstrings everywhere as requested. > > 246 + skip_check = ( > 247 + self.skip_check and > 248 + self.widgets[-1].value_from_datadict( > 249 + data, files, name + '_%s' % self.names[-1])) > > What significance is the -1th element? ie. add a comment here. Good point, done. > > 305 +class TestForm(forms.Form): > 306 + multi_field = testField > ... > 324 +class DictCharFieldTest(TestCase): > > This is a really small nit and it's not specific to this file, but I notice > that you name your test classes differently to what we normally have. No need > to rush around fixing everything but test classes usually *start* with "Test", > not *end* with that. Conversely, don't start non-test case classes with Test > :) In fact I'd use the prefix "Fake" instead of Test for those, e.g. FakeForm > instead of TestForm and then you'd be much more consistent with the rest of > the code we all maintain. Thanks for the advice, I've fixed this test file.