jama5262 / jiffy

Jiffy is a Flutter (Android, IOS and Web) date time package for parsing, manipulating, querying and formatting dates
https://pub.dev/packages/jiffy
MIT License
588 stars 126 forks source link

`fromNow` does not pluralize correctly: "1 minutes ago" #273

Open lukehutch opened 6 months ago

lukehutch commented 6 months ago

Describe the bug

Calling jiffy.fromNow() produces text that is not correctly pluralized in English (presumably also in other languages). For example: "1 minutes ago".

How to reproduce the bug

Call fromNow on a Jiffy from a minute ago.

What is the expected behavior

It should render the result differently if the number is 1, e.g. "1 minute ago" (same for minutes, hours, days, weeks, months, years).

Not every language needs special pluralization handling for the number 1, but a lot do.

jama5262 commented 6 months ago

Hey @lukehutch ,

I appreciate you bringing this to my attention.

I was able to replicate the issue


final j1 = Jiffy.parseFromList(
  [2024, 1, 1, 2, 0, 0, 0, 0]
);

final j2 = Jiffy.parseFromList(
    [2024, 1, 1, 1, 58, 30, 0, 0]
);

print(j1.from(j2)); // in 1 minutes

It should be in 2 minutes. Since the time difference its 1.5 minutes, and in Jiffy, we interpret that as in 2 minutes in relative time

Upon investigation, I've identified the issue. A recent pull request https://github.com/jama5262/jiffy/pull/267 addressed the usage of the diff function. However, there was a misunderstanding on my part regarding the asFloat parameter. I mistakenly interpreted it as indicating a time difference without decimal points, when in fact, it signifies a time difference with floating points. This misunderstanding was rectified in the mentioned pull request.

However, we overlooked updating the calculation of relative time in the from and fromNow functions in Jiffy, both of which rely on the diff function. Precision in floating-point values is crucial for accurately determining relative time. Therefore, we need to adjust the relative time calculation to accommodate floating-point time differences.

The solution involves updating the code to utilize floating-point values. This adjustment can be achieved by testing the asFloat parameter and setting it to true to calculate the relative time.

https://github.com/jama5262/jiffy/blob/a2788083d2b1180c48e72093acb41e7b2e940695/lib/src/display.dart#L47-L54

Thank you for your attention to this matter and your contribution to Jiffy.

I will open a PR to fix this.

lukehutch commented 6 months ago

Correct, asFloat: true gives fractional minutes, whereas asFloat: false gives integer minutes. As far as I know that is all working as intended, there is no PR needed there.

This is not the issue I am raising. The issue is that "1 minutes" is wrong, it should be localized in English as "1 minute". Similarly in Spanish it should be "1 minuto" not "1 minutos" etc. (I didn't check other languages).

The Jiffy source for fromNow purports to localize the results, and proper localization also requires proper pluralization.

jama5262 commented 6 months ago

Yes, correct, I was just saying the asFloat issue was fixed recently here https://github.com/jama5262/jiffy/pull/267, and that means we forgot to update the code here https://github.com/jama5262/jiffy/blob/a2788083d2b1180c48e72093acb41e7b2e940695/lib/src/display.dart#L47-L54

So the relative time calculation should use asFloat set to true and not false

Also , in Jiffy, there is no 1 minute its a minute

jama5262 commented 6 months ago

But other locales have different ways of representing one minute, in english its a minute

jama5262 commented 6 months ago

But the real issue that should be fixed is that we need to set the asFloat to true to calculate the relative time in Jiffy

jama5262 commented 6 months ago

In spanish, we have set it to un minuto, just as an example in other locales

https://github.com/jama5262/jiffy/blob/2795028883787d9e4563d47dd2d986091bb73c31/lib/src/locale/locales/es_locale.dart#L44

jama5262 commented 6 months ago

@lukehutch Here is the PR that should fix this big https://github.com/jama5262/jiffy/pull/274

lukehutch commented 6 months ago

Weird, I could have sworn I got "1 minutes ago" out of Jiffy.fromNow when I filed the bug. I must have been using an old version of Jiffy then, because I can't replicate it now. Glad I helped you find another bug though! :-)

lukehutch commented 6 months ago

@jama5262 OK, I took a look at the code, and I see what you were saying. But there is still a potential bug here, I think...

    if (seconds < 45) {
      result = relativeDateTime.lessThanOneMinute(seconds.round());
    } else if (seconds < 90) {
      result = relativeDateTime.aboutAMinute(minutes.round());
    } else if (minutes < 45) {
      result = relativeDateTime.minutes(minutes.round());

Even with everything in floating point, it is dicey to assume that if (seconds < 90) will "round the same way as" minutes.round().

The if (seconds < 90) should be if (minutes.round() == 1) to ensure that the singular form minute is only ever used with the number 1, and the plural form is used with everything else.

lukehutch commented 6 months ago

Same for hours, days, months, and years too, for the "about a" versions... they should all check the appropriate rounded version for consistency.

(In particular, months as a fractional number worries me -- what is a fractional month, when the length of a month changes from day to day?)

jama5262 commented 5 months ago

Hmm, ok, I see what you are trying to say

jama5262 commented 5 months ago

You you be able to setup a PR for this @lukehutch ?

lukehutch commented 5 months ago

@jama5262 Unfortunately no, I'm working up to 20 hours a day right now trying to get my app launched...