Add separator to FieldList#694
Conversation
| self.last_index = index | ||
| name = "%s-%d" % (self.short_name, index) | ||
| id = "%s-%d" % (self.id, index) | ||
| name = self.short_name + self._separator + str(index) |
There was a problem hiding this comment.
This appears to be confusing in the interpolation and operation of concatenation. Can you make this more explicit in the way it was?
There was a problem hiding this comment.
Sure thing, but may I ask why interpolation is more explicit in this case?
Once the separator is a string variable as well the interpolation is "%s%s%d" which isn't particularly explicit either, I would argue.
There was a problem hiding this comment.
A couple things:
+operator is used on floats, ints, strings etc. What are the rules and order of precedence when adding two different types ? In JS, `1 + " 2" === "1 2", whereas in Python thats a type error.- the type of
nameisn't explicit with your change, it is clearly a string when doing"%s-%d"
There was a problem hiding this comment.
I just saw that the minimum python version is 3.6 so we can use f-strings.
Would you agree that f"{self.short_name}{self._separator}{index}" is the nicest syntax?
| name = "%s-%d" % (self.short_name, index) | ||
| id = "%s-%d" % (self.id, index) | ||
| name = self.short_name + self._separator + str(index) | ||
| id = self.id + self._separator + str(index) |
| self.last_index = -1 | ||
| self._prefix = kwargs.get("_prefix", "") | ||
| self._separator = separator | ||
| self._field_separator = unbound_field.kwargs.get("separator", "-") |
There was a problem hiding this comment.
Is there a time where _separator and _field_separator would diverge in value?
There was a problem hiding this comment.
Yes, see test_enclosed_subform_mixed_separators for example. When assigning a separator to the contained form field but not to the containing list these fields diverge.
|
The implementation looks good so far, I added some comments to on the PR. Perhaps might be best to unify the two new attributes under the "separator" and drop the "field_separator". What do you think? |
|
See my comment above, as far as I can tell there is no nice way of unifying those two in case of mixed separators. |
azmeuk
force-pushed
the
feat/list-seperator
branch
from
cf56f36 to
89829cc
Compare
|
Thank you for your contribution! |
3 participants
As of right now there is no way of creating fully custom names and ids for nested forms that contain a field list since the
FieldListcontains a hardcoded separator. This PR adds the ability to set a custom separator for aFieldListsimilar toFormField.This PR also fixes #681 and adds the relevant tests to verify the fix works.
For mixed separators (see test
test_enclosed_subform_list_separatorandtest_enclosed_subform_mixed_separators) to work properly, the separator of the unbound field (in case it is a form field or another list) needs to be determined. For this we look into thekwargsused to construct the unbound field or default to-.