Skip to content
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

fields: keep entries order in errors list #258

Closed
wants to merge 4 commits into from

Conversation

the-tosh
Copy link

No description provided.


chain = itertools.chain(self.validators, extra_validators)
self._run_validation_chain(form, chain)

return len(self.errors) == 0
return not error_occured
Copy link
Contributor

@valtron valtron Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ignores errors from _run_validation_chain (hence the failing test). What about return all(e is None for e in self.errors)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. Yes, it will work, I guess.
Should I add the changes to this pull request? I'm still bad with pull requests, so I'm not sure it's even possible in technical meaning :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can add more changes, it's as simple as pushing to the-tosh:master 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about this, but my initial comment had a wrong suggestion :( The line should be return all(e is None for e in self.errors).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The funniest thing here, that I've been already about typing all... and then I just copypaste line from your message :) Well, fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to point out here that adding None to a list is actually adding something, but in this case its of NoneType. The else block in like 925 is doing more work for no good reason.

@whb07
Copy link
Contributor

whb07 commented Jun 12, 2018

Thanks for trying to contribute! Having said that this PR isn't refining what it appears to be doing. Adding None to the errors list and then having to do more work to check for the thing we just added is unnecessary.

As of now I'm going to be closing the PR. If you happen to find anything that you'd like to help out with dont hesitate to submit another PR.

@whb07 whb07 closed this Jun 12, 2018
@davidism
Copy link
Member

To clarify, I think what we're saying is that the issue can be solved by rendering the errors for the subfields individually. Perhaps you're using a template macro and it can be adjusted to understand nested fields.

@valtron
Copy link
Contributor

valtron commented Jun 12, 2018

@whb07 Looking at FieldList.errors (or .errors of any field enclosure containing it), it's impossible to determine which list item has errors. Code:

from multidict import MultiDict
from wtforms import FieldList, validators, Form, TextField

class TestForm(Form):
	test_field = FieldList(TextField("Test", [validators.required()]))

data = MultiDict([('test_field-{}'.format(i), value) for i, value in enumerate(['a', ''])])
form = TestForm(data)
assert not form.validate()

print(form.errors)
# This would imply list item 0 has an error:
# {'test_field': [['This field is required.']]}

for f in form.test_field: print(f.errors)
# []
# ['This field is required.']

@davidism It's possible to work around it, like you say. But the problem then becomes that FieldList.errors is only usable for detecting and counting errors somewhere in the list, but not identifying where they are. (Contrast with FormField, where there's no problem because subfields are keyed.)

My use case is AJAX form validation. I respond with form.errors and zip down it client-side to show errors in the right place.

@whb07
Copy link
Contributor

whb07 commented Jun 12, 2018

Okay this newest message shows me what you're actually trying to say. I do think that this issue of the FieldList not keeping track of the data passed to it is something that should be worked on. Having said that, the current PR as it stands is not doing that.

So clearly when you pass in the data such as test_field-0-name_of_field the form is correctly associating the data to the field. But you want it to return which specific field it was correct?

If thats your goal, I'd like to help out. Open a new PR with possible changes and tests verifying what you are talking about. I'll be happy to take a look at it as I've also run into this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants