-
Notifications
You must be signed in to change notification settings - Fork 966
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
Timespan.humanize() shows weird results #651
Comments
With the parameters you specify, you're calling this method:
As you can see, the maxUnit defaults to weeks, so if you want to have months, you'll have to pass the maxUnit explicitly. In addition, the first parameter indicates that you only want a maximum of two different time units returned. This means that you'll only get strings like |
Sorry, not getting a result of months was not the real issue here. |
Ah, sorry. Yeah I see what you mean now. It seems to be a bug in Humanizer. When the TimeSpan to humanize is longer than a month, the days that do not make up a complete month are counted doubly. In your example with 45 days, it should actually just print 6 weeks, 3 days. |
Ok, so now I just wait till version 2.3 is released? Or do I need to open a ticket somewhere? How does this work exactly? |
You've opened a "ticket" by posting this issue already :) I can take a shot at fixing this, but you'll have to keep an eye on this issue to see when it's implemented.. |
I need a fix to this issue for a project i'm working on so have created a branch and have written some tests that confirm the issue. Ill hopefully have a fix to this issue in a PR shortly. |
I've spent a day writing a fix for this issue and have arrived at an impasse. If anyone is interested in carrying my work on, I'd be happy to publish my branch. I'll provide some details here for anyone who would like to continue. As @Alexander882 has stated weeks can span longer than a month and so in scenarios of weeks > 4 the remainder is counted incorrectly. On my branch I changed the implementation to make the calculations based on the remaining timespan instead of the full timespan each time. So for example if working out 59 days with precision = 2 you would make the weeks calculation 59 / 7 = 8 and then subtract 8 * 7 = 56 from 59 = 3 when calculating the days to get the correct answer of "8 weeks, 3 days" This all worked pretty well and passed my unit tests but caused failures in existing unit tests. This is because the existing implementation doesn't calculate weeks in some scenarios and instead accumulates weeks and days together. So for example 405 days will give a result of "1 year, 1 month, 8 days". My implementation on the other hand results in "1 year, 1month, 1 week, 1 day" Taking this further there is a precision method that simply takes the not null values from the enumeration and simply takes the first (precision) values. So if the precision was set to three in my solution the day is lost and results in 404, 405, 406, 407, 408, 409, 410 all giving the same result of "1 year, 1 month, 1 week" To fix this I could change the precision method to check if precision is less than the not null time values in the enumeration and then combine where necessary to get the desired result. At this stage though the changes are significant enough that it probably needs a major contributor to take it further. |
@schnitty thanks for taking a stab at this and explaining the issue so clearly. It's appreciated. This is obviously a breaking change; but one I'd be happy to introduce as it's going to fix a bug. Please submit a PR from your branch and I will take over. Thanks. |
Just saw this, will publish in the next few days. |
Is this still not fixed? I'm having the same issue with version 2.2.0. |
I'm getting this too on 2.2.0. Humanizing 5127 days is giving me "732 weeks, 13 days" instead of "732 weeks, 3 days". |
Yep, still a problem here. Setting |
Still present on 2.5.1 - another workaround is to set maxUnit: TimeUnit.Year. BTW - great library 👍 |
There was a PR for this (#681), but it has been closed due to age. I will take a look this week at recreating it for the latest branch and see if we can get it merged and fixed. (Note for contributors, I will also fix unit tests to make sure everything passes and document any changes with outputs) |
I recently experienced the same problem and, before checking whether anyone was working on it, took it upon myself to fix it. Which I did. Do you mind, @AKTheKnight, if I submit a PR? I don't want to rain on anyone's parade, but it seems you haven't worked on your own (draft) PR in over a year... |
Hi @louis-z Absolutely no complaints from me if you submit your PR. Apologies I didn't get mine completed and merged to avoid the issue for you |
Hi,
An application I had to take over uses Humanizer and is showing strange results in some cases when using a Timespan.
I'm not familiar with Humanizer and I tried finding a similar issue and/or solution online but there was none so here I am.
'when using this code and with a timespan of for example 45 days the humanized text results in descriptions like "6 weeks, 14 days"
I would have expected 1 months, x weeks, x days, or in this case 8 weeks.
Is this a bug or a wrong usage of the Humanize function?
I'm using v2.2. from NuGet
The text was updated successfully, but these errors were encountered: