Opened 14 years ago

Closed 12 years ago

Last modified 12 years ago

#12467 closed New feature (fixed)

More helpful error message when loading fixture with invalid date

Reported by: Knut Nesheim Owned by: Raúl Cumplido
Component: Core (Serialization) Version: dev
Severity: Normal Keywords:
Cc: raulcumplido@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It would be extremely helpful to have a more helpful error message when loading a fixture an invalid date. This would allow me to actually see the invalid data and I may then correct the data.

Attached is a very simple patch that will include the invalid data in the exception string. It would be extremely helpful to see more data, like the model and pk.

Attachments (3)

12467.diff (684 bytes ) - added by Knut Nesheim 14 years ago.
12467.2.diff (6.6 KB ) - added by Julien Phalip 12 years ago.
12467.3.diff (6.7 KB ) - added by Julien Phalip 12 years ago.

Download all attachments as: .zip

Change History (14)

by Knut Nesheim, 14 years ago

Attachment: 12467.diff added

comment:1 by Russell Keith-Magee, 14 years ago

Component: Database layer (models, ORM)Serialization
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Matt McClanahan, 13 years ago

Severity: Normal
Type: New feature

comment:3 by Raúl Cumplido, 12 years ago

Cc: raulcumplido@… added
Easy pickings: unset
Owner: changed from nobody to Raúl Cumplido
Status: newassigned
UI/UX: unset

comment:4 by Raúl Cumplido, 12 years ago

Needs tests: unset

Pull Request provided here at github with tests.

comment:5 by Julien Phalip, 12 years ago

The patch looks good. While we're at it, we should also do the same for DateField's "invalid_date" and DateTimeField's "invalid" messages. See amended patch attached.

by Julien Phalip, 12 years ago

Attachment: 12467.2.diff added

in reply to:  5 comment:6 by Simon Charette, 12 years ago

Replying to julien:

The patch looks good. While we're at it, we should also do the same for DateField's "invalid_date" and DateTimeField's "invalid" messages. See amended patch attached.

That would be a great addition. I have one question concerning the attached patch.

How are the ValueError risen by datetime.date calls supposed to be translated? I see you're calling _(str(e)) before formating the invalid_date message but I can't find where django is supposed to catch i.e. _('month must be in 1..12') when makemessages logic is invoked.

comment:7 by Julien Phalip, 12 years ago

That's a good question. One way would be to purposely trigger exceptions while the module is loaded to gather all the various error messages from datetime.datetime(), but that feels dirty... This is also dangerous as there's no guarantee that the messages will be the same on every version of Python.

The most complete way would be for Django to do the same tests as datetime.datetime() and then provide its own messages. However, this wouldn't be very robust and would be annoying to maintain.

Perhaps the safest way is to simply not give any granular error message and just say that it's an invalid date, as suggested in the new patch attached.

by Julien Phalip, 12 years ago

Attachment: 12467.3.diff added

comment:8 by Julien Phalip, 12 years ago

Resolution: fixed
Status: assignedclosed

In [16966]:

Fixed #12467 -- Made the model data validation for DateField and DateTimeField more useful by actually telling what was the value that failed. Also did a bit of PEP8 cleanup in the area. Thanks to knutin for the report, to raulcd for the initial patch and to charettes for the review.

comment:9 by Anssi Kääriäinen, 12 years ago

The commit made a couple of always True asserts:

/home/akaj/Django/django_test/django/db/models/fields/__init__.py:513: SyntaxWarning: assertion is always true, perhaps remove parentheses?
  assert (kwargs.get('primary_key', False) is True,
/home/akaj/Django/django_test/django/db/models/fields/__init__.py:539: SyntaxWarning: assertion is always true, perhaps remove parentheses?

comment:10 by Julien Phalip, 12 years ago

Well spotted. This is weird, I didn't see those warnings when running the tests. I'll fix that in a minute. Thanks!

comment:11 by Julien Phalip, 12 years ago

In [16971]:

Fixed a couple of assert syntax warnings mistakingly introduced in r16966 and added some tests to prevent future regressions. Thanks to Anssi Kääriäinen for the report.
Refs #12467.

Note: See TracTickets for help on using tickets.
Back to Top