#33879 closed Cleanup/optimization (fixed)
timesince - wrong results for 11 months + several weeks
Reported by: | אורי | Owned by: | GianpaoloBranca |
---|---|---|---|
Component: | Utilities | Version: | dev |
Severity: | Normal | Keywords: | |
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 (last modified by )
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).
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).
Change History (19)
comment:1 by , 2 years ago
Description: | modified (diff) |
---|
comment:2 by , 2 years ago
Component: | Uncategorized → Utilities |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
Version: | 4.0 → dev |
comment:3 by , 2 years ago
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.
comment:4 by , 2 years ago
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.
comment:5 by , 2 years ago
You might want to take a look at this answer, and then maybe convert the days to weeks and days.
follow-up: 7 comment:6 by , 2 years ago
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.
comment:7 by , 2 years ago
Replying to אורי:
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 * 7And 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 comment).
comment:8 by , 2 years ago
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
comment:9 by , 2 years ago
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
comment:10 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 2 years ago
Has patch: | set |
---|
comment:12 by , 2 years ago
Needs tests: | set |
---|
comment:13 by , 2 years ago
Needs tests: | unset |
---|
comment:14 by , 2 years ago
Patch needs improvement: | set |
---|
The 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.
comment:15 by , 2 years ago
I updated the function with a different algorithm that covers cases related to month duration that were not covered by the month average.
comment:16 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:17 by , 23 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Tentatively accepting, we might certainly improve this calculation a bit.
"\u00A0" is the non-breaking space, as a line break between the digit and the text is bad behavior.