Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lukehutch opened this issue May 6, 2024 · 13 comments · Fixed by #274
Open

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

lukehutch opened this issue May 6, 2024 · 13 comments · Fixed by #274

Comments

@lukehutch
Copy link

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
Copy link
Owner

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 #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.

final seconds =
diff(firstDateTime, secondDateTime, Unit.second, false).abs();
final minutes =
diff(firstDateTime, secondDateTime, Unit.minute, false).abs();
final hours = diff(firstDateTime, secondDateTime, Unit.hour, false).abs();
final days = diff(firstDateTime, secondDateTime, Unit.day, false).abs();
final months = diff(firstDateTime, secondDateTime, Unit.month, false).abs();
final years = diff(firstDateTime, secondDateTime, Unit.year, false).abs();

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

I will open a PR to fix this.

@lukehutch
Copy link
Author

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
Copy link
Owner

Yes, correct, I was just saying the asFloat issue was fixed recently here #267, and that means we forgot to update the code here

final seconds =
diff(firstDateTime, secondDateTime, Unit.second, false).abs();
final minutes =
diff(firstDateTime, secondDateTime, Unit.minute, false).abs();
final hours = diff(firstDateTime, secondDateTime, Unit.hour, false).abs();
final days = diff(firstDateTime, secondDateTime, Unit.day, false).abs();
final months = diff(firstDateTime, secondDateTime, Unit.month, false).abs();
final years = diff(firstDateTime, secondDateTime, Unit.year, false).abs();

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
Copy link
Owner

jama5262 commented May 12, 2024

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

@jama5262
Copy link
Owner

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
Copy link
Owner

jama5262 commented May 12, 2024

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

String aboutAMinute(int minutes) => 'un minuto';

@jama5262
Copy link
Owner

@lukehutch Here is the PR that should fix this big #274

@lukehutch
Copy link
Author

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
Copy link
Author

lukehutch commented May 12, 2024

@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
Copy link
Author

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 jama5262 reopened this May 14, 2024
@jama5262
Copy link
Owner

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

@jama5262
Copy link
Owner

jama5262 commented May 14, 2024

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

@lukehutch
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants