Fixed #31506 -- Documented DateField and timedelta behavior with ExpressionWrapper.#19947
Conversation
|
Hello! Thank you for your contribution 💪 As it's your first contribution be sure to check out the patch review checklist. If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket! If you have any design or process questions then you can ask in the Django forum. Welcome aboard ⛵️! |
|
https://github.com/django/django/actions/runs/18447405998/job/52556055766?pr=19947 |
Per the contributing docs on spelling check, I think it would be correct to enclose them in double grave accents (``), as the first alternative suggests. |
There was a problem hiding this comment.
Thanks for the PR! Will be great to add help here to prevent paper cuts. 👍
A concern I have is that adding this level of detail to the ExpressionWrapper section could reinforce a misunderstanding about what it does.
The problem surfaces when using simpler expressions like F(), as in your example. The reporter expected this problem to be solved via ExpressionWrapper, but that was a misunderstanding. ExpressionWrapper only helps you set output_field, which is an ORM concept[0], not a database concept, on expressions that don't have output_field. If we are going to add admonitions to a tool's docs about when not to use that tool, I'd prefer the advice be general instead of highly specialized to date fields.
The advice you present here to use Cast or integer arithmetic or custom fields is correct, but we could present that in the DateField docs instead, where there are similar warnings about other paper cuts, rather than detouring into ExpressionWrapper.
That said, I do think we should clarify in the ExpressionWrapper docs that it doesn't do any casting in the database.
So, to pull it all together, my recommendation is to:
- clarify in
ExpressionWrapperhow it differs fromCast - move the advice about creating a custom model field to the docs for
output_field, where it suggestsoutput_fieldis "only required when Django is unable to automatically determine the result’s field type". That's true, but it is also helpful in cases where you want to encapsulate the python-side casting behavior, which you can do with custom fields, which you point out here very well. - move the admonition about postgres/mysql datefield arithmetic behavior to the
DateFieldadmonitions, keeping your integer arithmetic alternative example
Thanks again, and welcome aboard! ⛵
[0] - The fact that in at least one case output_field actually entails a database cast is a bug that people are likely relying on!
|
Hi @jacobtylerwalls, sorry for the late update! |
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Thank you, updates look great, just some last edits. Also mind fixing the merge conflicts?
@jacobtylerwalls Of course! I’ll take a close look at your comments. Thanks a lot:) |
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Thank you @forwardyoung! Should be good to go once the missing backticks and line lengths lints are fixed 👍
|
@jacobtylerwalls |
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Yay, checks are passing now. I gave this a final read and noticed one slight inconsistency, can you let me know if you agree with this suggestion?
…abase casts. Added warning in DateField documentation about type differences when using timedelta on PostgreSQL and MySQL. Mentioned Cast() and integer arithmetic solutions.
78cdb81 to
1dfa8b5
Compare
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Great, the latest rounds of changes read perfectly. Thanks for being so responsive. Welcome aboard! ⛵
|
@jacobtylerwalls It’s all thanks to you:) |
Add warning in ExpressionWrapper documentation about date/datetime type differences when using timedelta on PostgreSQL and MySQL. Included Cast() and integer arithmetic solutions.
Trac ticket number
ticket-31506
Branch description
It documents the behavioral differences across database backends when using DateField with timedelta inside an ExpressionWrapper.
To address the issue where arithmetic operations on date/time fields in PostgreSQL and MySQL can return a datetime type instead of a date, a warning has been added to the ExpressionWrapper documentation. It also provides solutions, such as using the Cast() function, to help developers avoid unintended type mismatches.
Checklist
mainbranch.