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

[BUG] Timespan Humanize MinUnit does not match documentation #873

Open
pmunin opened this issue Oct 31, 2019 · 5 comments
Open

[BUG] Timespan Humanize MinUnit does not match documentation #873

pmunin opened this issue Oct 31, 2019 · 5 comments

Comments

@pmunin
Copy link

pmunin commented Oct 31, 2019

The documentation says:

In addition, a minimum unit of time may be specified to avoid rolling down to a smaller unit. For example:
TimeSpan.FromMilliseconds(122500).Humanize(minUnit: TimeUnit.Second) => "2 minutes, 2 seconds" // instead of 2 minutes, 2 seconds, 500 milliseconds

When you run it on latest version you get different actual result:
TimeSpan.FromMilliseconds(122500).Humanize(minUnit: Humanizer.Localisation.TimeUnit.Second) => "2 minutes"

@pmunin pmunin changed the title Timespan Humanize MinUnit does not match documentation [BUG] Timespan Humanize MinUnit does not match documentation Nov 7, 2019
@trevynmace
Copy link

I'll take this one :)

@trevynmace
Copy link

Okay so it looks like the precision is the problem. The default precision for the Humanize call is 1, which then removes the rest of the data, in this case "2 seconds". If you make the call with precision: 2 as a parameter then it will work as the documentation specifies, however this still means that either the documentation is wrong, or the method itself is wrong. In my opinion, I think the method is incorrect and that the minUnit should not also require a precision parameter. I'm going to make the changes to fix it although if someone who maintains the project feels otherwise then I hope they chime in on our conversation :) Thanks!

@pmunin
Copy link
Author

pmunin commented Nov 13, 2019

precision will not work the same way. I want to have minimum unit set, not precision.
E.g.:
Expected
TimeSpan.FromMilliseconds(1000*(10+10*60)).Humanize(minUnit: TimeUnit.Second) => "10 minutes, 10 seconds".
TimeSpan.FromMilliseconds(1000*(10+10*60 + 1*60*60)).Humanize(minUnit: TimeUnit.Second) => "1 hour, 10 minutes, 10 seconds".

With precision=2 the second test case will return 1 hour, 10 minutes, when I need "seconds" for both cases per documentation.

@trevynmace
Copy link

Sorry, let me clarify. I was just giving an example of how a workaround would be possible by specifying the desired precision. I'll still work on a fix but I can't provide an ETA currently.

@pmunin
Copy link
Author

pmunin commented Nov 13, 2019

I see, well in this case I'll have to figure out myself which precision to use based on the value of timestamp. Thanks for your help though.

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

No branches or pull requests

2 participants