Django

Code

Ticket #7443 (closed: fixed)

Opened 6 months ago

Last modified 3 months ago

timesince template filter reverses its parameters when given a base date

Reported by: Andrew Shearer <ashearerw@shearersoftware.com> Assigned to: nobody
Milestone: Component: Template system
Version: SVN Keywords: timesince timeuntil date handling filter
Cc: Triage Stage: Ready for checkin
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

timesince.diff (1.1 kB) - added by Andrew Shearer <andrew@ashearer.com> on 06/13/08 01:38:31.
Patch
7443-r8486.diff (4.1 kB) - added by russellm on 08/23/08 10:07:12.
Patch applied as [8481] but then reverted.

Change History

06/13/08 01:38:31 changed by Andrew Shearer <andrew@ashearer.com>

  • attachment timesince.diff added.

Patch

(follow-up: ↓ 4 ) 06/13/08 01:40:47 changed by Andrew Shearer <andrew@ashearer.com>

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

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'

06/14/08 01:27:00 changed by anonymous

  • stage changed from Unreviewed to Ready for checkin.

06/14/08 01:27:18 changed by Simon Greenhill

(that was me)

(in reply to: ↑ 1 ) 06/18/08 06:50:34 changed by russellm

  • status changed from new to closed.
  • resolution set to invalid.

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.

(follow-up: ↓ 6 ) 08/23/08 01:44:28 changed by ashearer

  • status changed from closed to reopened.
  • resolution deleted.

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 ) 08/23/08 07:47:53 changed 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 :-)

08/23/08 07:52:05 changed by russellm

  • status changed from reopened to closed.
  • resolution set to fixed.

(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@shearersoftware.com> for the report.

08/23/08 10:06:37 changed by russellm

  • status changed from closed to reopened.
  • resolution deleted.

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.

08/23/08 10:07:12 changed by russellm

  • attachment 7443-r8486.diff added.

Patch applied as [8481] but then reverted.

08/23/08 22:14:01 changed 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.

08/25/08 07:38:21 changed by russellm

  • status changed from reopened to closed.
  • resolution set to fixed.

(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@shearersoftware.com> for the report.

Fat fingers stuffed up the commit message in SVN.


Add/Change #7443 (timesince template filter reverses its parameters when given a base date)




Change Properties
Action