[Django] #33879: timesince - wrong results for 11 months + several weeks
Django
-----------------------------------------+------------------------
Reporter: אורי | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Hi,
I'm using `timesince` to format how much time passed since the user last
visited my website. The code is:
{{{
_("On {date} ({timesince} ago)").format(
date=formats.date_format(value=last_visit_date),
timesince=timesince(d=last_visit_date, now=today)
)
}}}
Now I created a test to test these times, and I noticed that for a year
minus a week, the result is "(11\u00A0months, 4\u00A0weeks ago)" (why the
"\u00A0" and not a space?), and for a year minus 2 weeks, the result is
"(11\u00A0months, 3\u00A0weeks ago)":
{{{
user_18 = ActiveUserFactory()
user_18.profile.last_visit -= (relativedelta(years=1) -
relativedelta(weeks=1))
user_18.save_user_and_profile()
self.assertIs(expr1={'en': "(11\u00A0months, 4\u00A0weeks
ago)", 'he': "(לפני 11\u00A0חודשים, 4\u00A0שבועות)"}[self.language_code]
in user_18.profile.last_visit_str, expr2=True)
user_19 = ActiveUserFactory()
user_19.profile.last_visit -= (relativedelta(years=1) -
relativedelta(weeks=2))
user_19.save_user_and_profile()
self.assertIs(expr1={'en': "(11\u00A0months, 3\u00A0weeks
ago)", 'he': "(לפני 11\u00A0חודשים, 3\u00A0שבועות)"}[self.language_code]
in user_19.profile.last_visit_str, expr2=True)
}}}
Now, a year is 365 days, a year minus one week is 358 days, which is 11
months and 3 weeks. I think the problem is because each month is
considered as 30 days, so 11 months are 330 days. But 11 months are about
334 days actually, so we receive a result of 11 months and 4 weeks,
instead of 11 months and 3 weeks.
A fix would be to change the number of days in a month to 30.4 (the
average), optionally only for more than 2 months (because it makes sense
to calculate exactly 30 days for the first 2 months).
--
Ticket URL: <https://code.djangoproject.com/ticket/33879>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: אורי | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by אורי:
Old description:
New description:
Hi,
Also, it's important to calculate the number of days in 11 (or any number)
of months as an integer, so that the result will not display hours and
minutes (if `depth` is big enough).
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:1>
Django
Reporter: אורי | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* type: Uncategorized => Cleanup/optimization
* version: 4.0 => dev
* component: Uncategorized => Utilities
* stage: Unreviewed => Accepted
Comment:
Tentatively accepting, we might certainly improve this calculation a bit.
> (why the "\u00A0" and not a space?)
"\u00A0" is the non-breaking space, as a line break between the digit and
the text is bad behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: אורי | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by אורי):
Thank you, Claude. I suggest calculating the number of days per month:
{{{
If months <= 2:
30 * months
else:
int(30.4 * months)
}}}
(I confirm that `int(30.4 * months) == 30 * months` if `months` is 1 or 2)
I'm not sure if I know how to submit a PR since currently
`TIMESINCE_CHUNKS` doesn't support this. And I don't know how to change
the algorithm to support this.
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: אורי | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by אורי):
Hi Claude,
I just looked at the algorithm and I found another bug:
If I add to my test this code:
{{{
user_21 = ActiveUserFactory()
user_21.profile.last_visit -= (relativedelta(years=2) -
relativedelta(days=1))
user_21.save_user_and_profile()
import re
print(re.sub(r'[^ -~]', lambda m: '\\u%04X' % ord(m[0]),
user_21.profile.last_visit_str))
user_22 = ActiveUserFactory()
user_22.profile.last_visit -= (relativedelta(years=2) -
relativedelta(days=2))
user_22.save_user_and_profile()
import re
print(re.sub(r'[^ -~]', lambda m: '\\u%04X' % ord(m[0]),
user_22.profile.last_visit_str))
}}}
Then, I get printed:
{{{
On 1 August 2020 (1\u00A0year, 12\u00A0months ago)
On 2 August 2020 (1\u00A0year, 12\u00A0months ago)
}}}
(In English)
So it returns 1 year, 12 months ago in both cases. Also I guess with 364
days it will return 12 months and not 1 year.
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: אורי | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by אורי):
You might want to take a look at
[https://stackoverflow.com/a/50812971/1412564 this answer], and then maybe
convert the days to weeks and days.
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: אורי | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by אורי):
You can calculate something like this:
{{{
diff = relativedelta(date2, date1)
years = diff.years
months = diff.months
weeks = diff.days // 7
days = diff.days - weeks * 7
}}}
And then calculate the minutes and seconds from `delta.seconds` in the
original function.
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: אורי | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:6 אורי]:
> You can calculate something like this:
>
> {{{
> from dateutil.relativedelta import relativedelta
>
> diff = relativedelta(date2, date1)
>
> years = diff.years
> months = diff.months
> weeks = diff.days // 7
> days = diff.days - weeks * 7
> }}}
>
> And then calculate the hours and minutes from `delta.seconds` in the
original function.
Adding a new dependency is not an option, IMO. However you can always
start a discussion on the DevelopersMailingList (see
[https://code.djangoproject.com/ticket/32727#comment:11 comment]).
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: אורי | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by אורי):
Hi,
I created my own utility function:
{{{
from dateutil.relativedelta import relativedelta
from django.utils.timesince import TIME_STRINGS as timesince_time_strings
from django.utils.html import avoid_wrapping
from django.utils.translation import gettext, get_language
def timesince(d, now):
delta = relativedelta(now, d)
years = delta.years
months = delta.months
weeks = delta.days // 7
days = delta.days - weeks * 7
timesince_counts = [(years, "year"), (months, "month")]
if (years == 0):
timesince_counts.append((weeks, "week"))
if (months == 0):
timesince_counts.append((days, "day"))
result = []
for (count, name) in timesince_counts:
if (count > 0):
result.append(avoid_wrapping(value=timesince_time_strings[name] % {"num":
count}))
return gettext(", ").join(result)
}}}
I don't need depth>2, I don't need hours and minutes and by definition my
function returns "" if both dates are the same. now must be bigger than d
or else I don't know what will happen... I think you just get ""
otherwise.
https://github.com/speedy-net/speedy-
net/blob/master/speedy/core/base/utils.py
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:8>
Django
--------------------------------------+------------------------------------
Reporter: אורי | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by אורי):
Hi,
I updated my code a little:
{{{
from dateutil.relativedelta import relativedelta
from django.utils.timesince import TIME_STRINGS as timesince_time_strings
from django.utils.html import avoid_wrapping
from django.utils.translation import pgettext, get_language
def timesince(d, now):
"""
Like Django's timesince but more accurate. Returns results only when
delta is at least one day (positive). Otherwise returns "". Result is
either one or two in depth.
"""
delta = -relativedelta(d, now)
result = []
if ((delta.years >= 0) and (delta.months >= 0) and (delta.days >= 0)):
years = delta.years
months = delta.months
weeks = delta.days // 7
days = delta.days - weeks * 7
timesince_counts = [(years, "year"), (months, "month")]
if (years == 0):
timesince_counts.append((weeks, "week"))
if (months == 0):
timesince_counts.append((days, "day"))
for (count, name) in timesince_counts:
if (count > 0):
result.append(avoid_wrapping(value=timesince_time_strings[name] % {"num":
count}))
result = pgettext(context="timesince", message=", ").join(result)
if (get_language() == "he"):
result = re.sub(pattern=r'(\ {1}ו{1})(\d{1})', repl=lambda m:
"-".join(m.groups()), string=result)
return result
}}}
I also think you can consider using `dateutil.relativedelta` if `python-
dateutil` is installed, and adding it as an optional dependency on your
docs. And if not, revert to the current algorithm.
Notice, I used `-relativedelta(d, now)` since it displays more accurate
results for dates in the past, than `relativedelta(now, d)`. You can see
[https://github.com/dateutil/dateutil/issues/1228]
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:9>
Django
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GianpaoloBranca):
* owner: nobody => GianpaoloBranca
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by GianpaoloBranca):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
The [https://github.com/django/django/pull/16027 suggested patch] adds a
(I think) simple enough adjustment for varying month length. If that can
be encapsulated in a helper function (perhaps with a few tests) I think
should be ≈good to go.
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GianpaoloBranca):
I updated the function with a different algorithm that covers cases
related to month duration that were not covered by the month average.
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Component: Utilities | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8d67e16493c903adc9d049141028bc0fff43f8c8" 8d67e164]:
{{{
#!CommitTicketReference repository=""
revision="8d67e16493c903adc9d049141028bc0fff43f8c8"
Fixed #33879 -- Improved timesince handling of long intervals.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: אורי | Owner:
Type: | GianpaoloBranca
Cleanup/optimization | Status: closed
Component: Utilities | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"4593bc5da115f2e808a803a4ec24104b6c7a6152" 4593bc5]:
{{{
#!CommitTicketReference repository=""
revision="4593bc5da115f2e808a803a4ec24104b6c7a6152"
Refs #33879 -- Fixed plural value deprecation warnings.
Plural value must be an integer.
Regression in 8d67e16493c903adc9d049141028bc0fff43f8c8.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33879#comment:19>