Opened 3 years ago

Closed 2 months ago

#19210 closed Bug (fixed)

django.utils.timesince() does not account for leap years

Reported by: jnns Owned by: raphaelm
Component: Utilities Version: master
Severity: Normal Keywords: date time
Cc: hirokiky@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The output of django.utils.timesince() will become increasingly inaccurate as the number of leap years in the supplied time period grows.

Attachments (1)

19210_test.patch (734 bytes) - added by hirokiky 3 years ago.
Added test to verifiy the behavior while long time.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 3 years ago by claudep

  • Component changed from Uncategorized to Core (Other)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Using 365.2425 instead of 365 in chunks might give better results for long periods, without much distortion for shorter periods.

Changed 3 years ago by hirokiky

Added test to verifiy the behavior while long time.

comment:2 Changed 3 years ago by hirokiky

I applied my patch and tried test, using 365.2425 instead of 365 in chunks.
test_thousand_years_ago passed, but a test failed, like this:

======================================================================
FAIL: test_other_units (regressiontests.utils.timesince.TimesinceTests)
Test other units.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/hirokiky/django/django/tests/regressiontests/utils/timesince.py", line 42, in test_other_units
    self.assertEqual(timesince(self.t, self.t+self.oneyear), '1 year')
AssertionError: u'12 months' != u'1 year'
- 12 months
+ 1 year


----------------------------------------------------------------------

comment:3 Changed 3 years ago by hirokiky

Fixed test not to use dateutil https://github.com/hirokiky/django/commit/b3b81594fc6c0a424c414ffdc751e01c55e11cb1
Because, other sources doesn't use it.

comment:4 Changed 3 years ago by hirokiky

Solved with dirty implemantion https://github.com/hirokiky/django/commit/f581659080e78891d043ecbd63e2fc349c786c44
And commited to fix this commit https://github.com/hirokiky/django/commit/68d64fe91a73dbf4bebda98ebee0bb7b8b084ab6

It seems good to separate the resolusion depending on whether the unit is year or not.

Last edited 3 years ago by hirokiky (previous) (diff)

comment:5 Changed 3 years ago by hirokiky

  • Cc hirokiky@… added
  • Owner changed from nobody to hirokiky
  • Status changed from new to assigned

comment:6 Changed 3 years ago by hirokiky

  • Has patch set

comment:7 Changed 3 years ago by hirokiky

I send pull-request as https://github.com/django/django/pull/622 .

Please check it, if you do not mind.

comment:8 Changed 2 years ago by aaugustin

  • Component changed from Core (Other) to Utilities

comment:9 Changed 2 years ago by susan

@hirokiky, I've applied your PR branch and there are errors in the utils_tests test suite. See stack trace below:

======================================================================
FAIL: test_date_objects (utils_tests.test_timesince.TimesinceTests)
Both timesince and timeuntil should work on date objects (#17937).


Traceback (most recent call last):
../Projects/django/tests/utils_tests/test_timesince.py", line 112, in test_date_objects

self.assertEqual(timesince(today + self.oneday), '0\xa0minutes')

AssertionError: u'0 minutes' != u'0\xa0minutes'

  • 0 minutes

?
+ 0\xa0minutes
?

======================================================================
FAIL: test_different_timezones (utils_tests.test_timesince.TimesinceTests)
When using two different timezones.


Traceback (most recent call last):
../Projects/django/tests/utils_tests/test_timesince.py", line 105, in test_different_timezones

self.assertEqual(timesince(now), '0\xa0minutes')

AssertionError: u'0 minutes' != u'0\xa0minutes'

  • 0 minutes

?
+ 0\xa0minutes
?

======================================================================
FAIL: test_display_second_before_first (utils_tests.test_timesince.TimesinceTests)


Traceback (most recent call last):
../Projects/django/tests/utils_tests/test_timesince.py", line 74, in test_display_second_before_first

'0\xa0minutes')

AssertionError: u'0 minutes' != u'0\xa0minutes'

  • 0 minutes

?
+ 0\xa0minutes
?

======================================================================
FAIL: test_equal_datetimes (utils_tests.test_timesince.TimesinceTests)
equal datetimes.


Traceback (most recent call last):
../Projects/django/tests/utils_tests/test_timesince.py", line 25, in test_equal_datetimes

self.assertEqual(timesince(self.t, self.t), '0\xa0minutes')

AssertionError: u'0 minutes' != u'0\xa0minutes'

  • 0 minutes

?
+ 0\xa0minutes
?

======================================================================
FAIL: test_ignore_microseconds_and_seconds (utils_tests.test_timesince.TimesinceTests)
Microseconds and seconds are ignored.


Traceback (most recent call last):
../Projects/django/tests/utils_tests/test_timesince.py", line 30, in test_ignore_microseconds_and_seconds

'0\xa0minutes')

AssertionError: u'0 minutes' != u'0\xa0minutes'

  • 0 minutes

?
+ 0\xa0minutes
?

======================================================================
FAIL: test_naive_datetime_with_tzinfo_attribute (utils_tests.test_timesince.TimesinceTests)


Traceback (most recent call last):
.../django/tests/utils_tests/test_timesince.py", line 127, in test_naive_datetime_with_tzinfo_attribute

self.assertEqual(timesince(future), '0\xa0minutes')

AssertionError: u'0 minutes' != u'0\xa0minutes'

  • 0 minutes

?
+ 0\xa0minutes
?


Ran 248 tests in 0.473s

FAILED (failures=6, errors=6)

Did anyone else also get errors? Or did all the tests pass?

comment:10 Changed 2 years ago by timo

Susan, it sounds like you may be running the latest version of the tests with a version of Django before 7d77e9786a118dd95a268872dd9d36664066b96a (or perhaps you didn't apply the pull request quite right).

comment:11 Changed 2 years ago by susan

The 6-mo old patch was created before that new commit you're referring to, timo. That means the patch needs to be updated.

comment:12 Changed 2 years ago by susan

I partially updated the patch on my origin branch here: https://github.com/onceuponatimeforever/django/compare/pull_622. It's not a PR, because there are still errors. I say "partially updated", because when I run the utils_tests, I get a dozen errors related to this:

 File "../django/django/utils/timesince.py", line 65, in timesince
    s = ugettext('%(number)d %(type)s') % {'number': count, 'type': name(count)}
TypeError: '__proxy__' object is not callable

I'm not sure how to fix this error. But I'll leave it up to the next contributor to take a look at this.

comment:13 Changed 2 years ago by timo

  • Patch needs improvement set

comment:14 Changed 15 months ago by hirokiky

  • Owner hirokiky deleted
  • Status changed from assigned to new

Thanks @susan @timo. I can't take time for this problem, so I'm leaving.

comment:15 Changed 11 months ago by aaugustin

For what it's worth, I consider that timesince returns an order of magnitude for human consumption and a small inaccuracy isn't critical. I wouldn't mind if this ticket was closed as wontfix.

comment:16 Changed 11 months ago by jnns

What about the use case that I want to display the age of a person given his or her birthdate? I consider this an order of magnitude for human consumption but timesince will be displaying an incorrect number of years.

comment:17 Changed 2 months ago by raphaelm

  • Owner set to raphaelm
  • Status changed from new to assigned

comment:18 Changed 2 months ago by raphaelm

  • Patch needs improvement unset

After looking into a lot of algorithms for date diffing, including the ones suggested above and the ones from python-dateutil (which get pretty complex) I decided to go for the most simple solution I could find. I assume timesince wasn't ever meant to be an exact calculation. If we would want an exact calculation, we should completely re-implement it to correctly work with the different number of days in the months and so on.

However, there is a very simple fix to the accumulated error that grows with the given time period: just use calendar.leapdays from the standard library and substract it from the number of days.

I created a pull request including a regression test: https://github.com/django/django/pull/4785

Last edited 2 months ago by raphaelm (previous) (diff)

comment:19 Changed 2 months ago by Tim Graham <timograham@…>

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

In 6700c90:

Fixed #19210 -- Added leap year support to django.utils.timesince()

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