Opened 15 years ago
Closed 13 years ago
#11595 closed Cleanup/optimization (fixed)
Fixture validation errors should report their data
Reported by: | freyley | Owned by: | Raúl Cumplido |
---|---|---|---|
Component: | Core (Serialization) | Version: | 1.0 |
Severity: | Normal | Keywords: | easy-pickings |
Cc: | Anand Kumria, Peter van Kampen | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
For example, here's what django/db/models/fields/__init__.py
has for lines 341-348:
def to_python(self, value): if value is None: return value try: return int(value) except (TypeError, ValueError): raise exceptions.ValidationError(_("This value must be an integer."))
Changing line 348 to:
_("(%s) must be an integer." % value))
means that when you convert a text field to a joined object, you know which one's broken where. (Other places in that file do the same thing)
Attachments (2)
Change History (20)
comment:1 by , 15 years ago
Has patch: | unset |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 14 years ago
Description: | modified (diff) |
---|---|
Keywords: | easy-pickings added |
Severity: | → Normal |
Type: | → Cleanup/optimization |
comment:3 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 14 years ago
Description: | modified (diff) |
---|
comment:6 by , 14 years ago
The message proposed by the OP is a bit weird. If you insert the non-valid value to the error message you get something like:
(foobar) must be an integer
I'm a bit puzzled by the assumption, that this code is only used by serialization framework. It could've been true 14 months ago, but now it's a part of model validation. I don't want my users to see messages like these.
comment:7 by , 14 years ago
Do you think we had to close it? Maybe 21 months later since it was opened it makes no sense.
comment:8 by , 14 years ago
@raulcd -- What you have proposed sounds right to me. My only comment: you don't need to wrap the value in "_()". That marks a string for translation, which isn't something we can do with user input.
@lrekucki's comment is purely about the literal text for the message. The parentheses in "(foobar) must be an integer" is a strange construction. '"foobar" must be an integer' would make more sense.
comment:9 by , 14 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:10 by , 14 years ago
Needs tests: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
@raulcd -- As described in the contribution guide and HOWTO, you shouldn't mark your own patch as ready for checkin; that's for someone else to decide. We like to get at least 2 sets of eyes on a patch before marking it for commit.
In this case, your patch doesn't have tests, so it can't be ready for checkin.
comment:11 by , 14 years ago
@russellm -- I am sorry I misunderstood the documentation. I thought the meaning of the ready for chekin was set the ticket as "ey guys, can anyone take a look, I did my patch". I will know that for the next one. About the tests I executed the full test suite and as no test failed I thought the "literal" of the error may not be tested. I will prepare the tests and submit the new patch, just a question (as I am new) where do you think is the best location to create the tests? In /trunk/tests/modeltests/validation or create a new folder with the tests. I am sorry for this questions, but as a newbie I want to follow your standards.
by , 14 years ago
Attachment: | fixture_validation.diff added |
---|
Added their data to fixture validation errors and tests
comment:12 by , 14 years ago
Needs tests: | unset |
---|
I finally added tests to a new file. It would be better, as someone in the IRC told me, to move them if you think they fit better in another location. (tests/modeltests/validation/test_error_messages.py is the new test file).
As you told me I will not set the ready for chekin, just wait comments. I removed the need tests checkbox (I don't know if I had to do it or someone who review the patch has to do it).
comment:13 by , 14 years ago
Easy pickings: | set |
---|
comment:14 by , 13 years ago
UI/UX: | unset |
---|
The current patch does not apply cleanly to the tree, I am working on this as part of the DjangoCon sprint and will it up.
comment:15 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Is testing all cases that were fixed, applies cleanly to trunk
comment:16 by , 13 years ago
Cc: | added |
---|
comment:17 by , 13 years ago
Cc: | added |
---|
It seems the code is changed since the bug was reported. Now the code has dictionaries with default_error_messages. Example for lines 865-857:
And in the Exception it has the next, lines 884-890:
Should we change the code to:
I am new and that's why I taked an easy_pickings one. But I want to have your ok to prepare my first patch.