-
-
Notifications
You must be signed in to change notification settings - Fork 395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
name
isn't explicit with your change, it is clearly a string when doing"%s-%d"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok to me.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a time where _separator and _field_separator would diverge in value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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
FieldList
contains a hardcoded separator. This PR adds the ability to set a custom separator for aFieldList
similar 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_separator
andtest_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 thekwargs
used to construct the unbound field or default to-
.