Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#13282 closed (fixed)

Document patch: WEEK_DAY should start from 0, not 1

Reported by: wangchun Owned by: nobody
Component: Documentation Version: 1.1
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Confirmed with postgresql

Attachments (3)

13282.diff (1015 bytes) - added by wangchun 4 years ago.
13282-r13135.diff (939 bytes) - added by ramiro 4 years ago.
13282-r13135.2.diff (939 bytes) - added by timo 4 years ago.
ramiro's patch with # for Saturday fixed from 6 to 7

Download all attachments as: .zip

Change History (9)

Changed 4 years ago by wangchun

comment:1 follow-up: Changed 4 years ago by kmtracey

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

There is a slight problem in the doc here, but the patch does not fix it. The only thing wrong in the cited doc is that the "SQL equivalent" is not in fact the SQL produced, for any supported database, for the given example. For the function as implemented, the days of the week do start on Sunday and the numbering does starts at 1, as documented. So the parts of the patch which change that are incorrect. The example query, using week_day of 2, will in fact produce all matching objects that fall on a Monday.

The problem in the doc is that the extract(dow) function is what is used to implement the feature on PostgreSQL, and there the days are numbered differently. If you check the actual SQL generated for the example query, you will see that it adds 1 to the result of extract(dow) and compares against the requested numerical day of week. Putting the actual PostgreSQL query in the doc, would, I think, be more confusing than helpful. Putting the MySQL version of the query (DAYOFWEEK(pub_date) = 2) would likely be more helpful. Alternatively we could remove the "SQL equivalent" entirely and just describe what the function does without including any indication of the SQL generated (which in fact is very different for the different databases).

Note changing the numbering is not an option, this was already requested and rejected in #10345, and cannot be changed at this point since it would break backwards compatibility.

Changed 4 years ago by ramiro

comment:2 in reply to: ↑ 1 Changed 4 years ago by ramiro

  • Has patch set
  • milestone set to 1.2

Replying to kmtracey:

Alternatively we could remove the "SQL equivalent" entirely and just describe what the function does without including any indication of the SQL generated (which in fact is very different for the different databases).

I've attached a patch implementing just that choice as outlined by Karen.

Changed 4 years ago by timo

ramiro's patch with # for Saturday fixed from 6 to 7

comment:3 Changed 4 years ago by timo

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:4 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [13155]) Fixed #13282 -- Clarified documentation around week_day filtering in querysets. Thanks to wangchun, Ramiro Morales and timo.

comment:5 Changed 4 years ago by russellm

(In [13159]) [1.1.X] Fixed #13282 -- Clarified documentation around week_day filtering in querysets. Thanks to wangchun, Ramiro Morales and timo.

Backport of r13155 from trunk.

comment:6 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.