Code

Opened 6 years ago

Closed 6 years ago

#7443 closed (fixed)

timesince template filter reverses its parameters when given a base date

Reported by: Andrew Shearer <ashearerw@…> Owned by: nobody
Component: Template system Version: master
Severity: Keywords: timesince timeuntil date handling filter
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The timesince template filter appears to swap its parameters when given a base date argument, leading to a typical result of "0 minutes" instead of the expected time difference. This makes it a synonym of the timeuntil filter. Omitting the optional argument makes both filters work as expected.

The django.utils.timesince function itself works correctly. The timesince and timeuntil template filters both call it with an identical line of code, explaining their matching results.

A patch is attached that un-flips the parameters and adds test coverage of the base date argument to the filters.

Attachments (2)

timesince.diff (1.1 KB) - added by Andrew Shearer <andrew@…> 6 years ago.
Patch
7443-r8486.diff (4.1 KB) - added by russellm 6 years ago.
Patch applied as [8481] but then reverted.

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by Andrew Shearer <andrew@…>

Patch

comment:1 follow-up: Changed 6 years ago by Andrew Shearer <andrew@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Failing test result before applying the patch:

File "tests.py", line 354, in __main__
Failed example:
    timesince(datetime.datetime(2005, 12, 29), datetime.datetime(2005, 12, 30))
Expected:
    u'1 day'
Got:
    u'0 minutes'

comment:2 Changed 6 years ago by anonymous

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:3 Changed 6 years ago by Simon Greenhill

(that was me)

comment:4 in reply to: ↑ 1 Changed 6 years ago by russellm

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

Replying to Andrew Shearer <andrew@ashearer.com>:

Patch breaks the template filter test for timesince. The break occurs because the provided example of bad behaviour is incorrect:

Failed example:

timesince(datetime.datetime(2005, 12, 29), datetime.datetime(2005, 12, 30))

Expected:

u'1 day'

Got:

u'0 minutes'

29 Dec is before 30 december (the now argument on the timesince filter) - therefore 0 minutes is returned. The parameter ordering is slightly different to the timesince utility method.

comment:5 follow-up: Changed 6 years ago by ashearer

  • Resolution invalid deleted
  • Status changed from closed to reopened

I've re-read the documentation, and it seems to clearly contradict the current behavior. At least two of the three relevant sentences contradict it, while the remaining sentence supports it. If nothing else, the documentation should be changed.

But the current behavior still doesn't make much sense. Here's the output of a test to illustrate that.

!python
context = {
        'now': datetime.now(),
        'earlier': datetime.now() - timedelta(days=7),
        'later': datetime.now() + timedelta(days=7),
}

Template output:

  • earlier|timesince: 1 week
  • earlier|timesince:now: 0 minutes
  • earlier|timeuntil: 0 minutes
  • earlier|timeuntil:now: 0 minutes
  • later|timesince: 0 minutes
  • later|timesince:now: 1 week
  • later|timeuntil: 1 week
  • later|timeuntil:now: 1 week

Problem 1: mydate|timesince:now == mydate|timeuntil:now. The two-argument forms of timesince and timeuntil are synonyms, and therefore redundant. But their names, documentation, and single-argument forms suggest that they should give opposite results.

Problem 2: mydate|timesince != mydate|timesince:now. According to its documentation, timesince "[t]akes an optional argument that is a variable containing the date to use as the comparison point (without the argument, the comparison point is now)." But passing now explicitly as the comparison point gives opposite results from letting the comparison point default to now. (The timeuntil documentation contains almost exactly the same language, but passing now explicitly works the same as letting it default implicitly.)

Problem 3: The timesince documentation also says that '“0 minutes” will be returned for any date that is in the future relative to the comparison point.' The previous paragraph says that the optional argument is the comparison point. This corresponds to the 'later|timesince:now' line above, which returns '1 week' instead of the '0 minutes' documented.

In summary, the timesince filter is at best confusing (in which I have company at ticket #8453) and at worst flies in the face of analogies to timeuntil, its own single argument form, and two out of the three sentences of documentation that mention its two-argument form.

(By the way, russelm: the parameter ordering is "slightly" different? There are only two parameters. Which ordering would rise to the level of "extremely" different?)

comment:6 in reply to: ↑ 5 Changed 6 years ago by russellm

Replying to ashearer:

In summary, the timesince filter is at best confusing (in which I have company at ticket #8453) and at worst flies in the face of analogies to timeuntil, its own single argument form, and two out of the three sentences of documentation that mention its two-argument form.

Now that you've provided a set of test cases and highlighted the exact problem, I can see what you're driving at. If your original patch had these 8 examples (and then explained why the patch broke the three existing test cases), it would have been accepted much faster.

I'll update the patch and tests and commit this in the near future.

(By the way, russelm: the parameter ordering is "slightly" different? There are only two parameters. Which ordering would rise to the level of "extremely" different?)

You know - pedantry has a lot more impact when you take the care to correctly spell the name of the person you are criticising :-)

comment:7 Changed 6 years ago by russellm

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

(In [8481]) Fixed #7443: Corrected a long standing mistake in the timesince/timeuntil filters when using a parameter for 'now'. Thanks to Andrew Shearer <ashearerw@…> for the report.

comment:8 Changed 6 years ago by russellm

  • Resolution fixed deleted
  • Status changed from closed to reopened

I wasn't aware of #8453, which makes it seem like there has been some other discussions around this topic. I've reverted the change until I work out what is going on here. I'll attach the patch that I reverted in case we need to apply it again.

Changed 6 years ago by russellm

Patch applied as [8481] but then reverted.

comment:9 Changed 6 years ago by jcarbaugh

Just wanted to point out that this issue was also fixed in #7201 which fixes a timezone issue with timesince and timeuntil. I wasn't aware of this patch or #8453 when I fixed #7201 at the DC sprint. If fixing timesince in this patch causes backwards-incompatible behavior, it would make sense to commit it with #7201 as that also has backwards-incompatible changes.

comment:10 Changed 6 years ago by russellm

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

(In [8535]) Fixed #7743: Reverted [8483], which was itself a reversion of [8481], after confirmation from Malcolm. Corrected a long standing mistake in the timesince/timeuntil filters when using a parameter for 'now'. Thanks to Andrew Shearer <ashearerw@…> for the report.

Fat fingers stuffed up the commit message in SVN.

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.