Add separator to FieldList
#694
Conversation
src/wtforms/fields/core.py
Outdated
| @@ -1203,8 +1209,8 @@ def _add_entry(self, formdata=None, data=unset_value, index=None): | |||
| if index is None: | |||
| index = self.last_index + 1 | |||
| 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?
src/wtforms/fields/core.py
Outdated
| 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) |
| @@ -1104,6 +1108,8 @@ def __init__( | |||
| self.max_entries = max_entries | |||
| 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! |
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-.