Opened 16 years ago

Closed 16 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: dev
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: no UI/UX: no

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@…> 16 years ago.
Patch
7443-r8486.diff (4.1 KB ) - added by Russell Keith-Magee 16 years ago.
Patch applied as [8481] but then reverted.

Download all attachments as: .zip

Change History (12)

by Andrew Shearer <andrew@…>, 16 years ago

Attachment: timesince.diff added

Patch

comment:1 by Andrew Shearer <andrew@…>, 16 years ago

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 by anonymous, 16 years ago

Triage Stage: UnreviewedReady for checkin

comment:3 by Simon Greenhill, 16 years ago

(that was me)

in reply to:  1 comment:4 by Russell Keith-Magee, 16 years ago

Resolution: invalid
Status: newclosed

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 by ashearer, 16 years ago

Resolution: invalid
Status: closedreopened

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?)

in reply to:  5 comment:6 by Russell Keith-Magee, 16 years ago

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 by Russell Keith-Magee, 16 years ago

Resolution: fixed
Status: reopenedclosed

(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 by Russell Keith-Magee, 16 years ago

Resolution: fixed
Status: closedreopened

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.

by Russell Keith-Magee, 16 years ago

Attachment: 7443-r8486.diff added

Patch applied as [8481] but then reverted.

comment:9 by Jeremy Carbaugh, 16 years ago

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 by Russell Keith-Magee, 16 years ago

Resolution: fixed
Status: reopenedclosed

(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.

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