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

TimeSpan.Humanize does not behave as expected #583

Closed
cryptiklemur opened this issue Aug 19, 2016 · 16 comments
Closed

TimeSpan.Humanize does not behave as expected #583

cryptiklemur opened this issue Aug 19, 2016 · 16 comments

Comments

@cryptiklemur
Copy link

cryptiklemur commented Aug 19, 2016

WIth the following timespan of: 296.19:32:06.8100684

Using the following code:

string createdAt = timeSpan.Humanize(10,  minUnit: TimeUnit.Day, maxUnit: TimeUnit.Year);

I would expect the output to be: Something around 9 months, etc. But instead, it shows 2 days

@hangy
Copy link
Contributor

hangy commented Aug 19, 2016

Apparently not implemented, yet:

private const int _lastTimeUnitTypeIndexImplemented = (int)TimeUnit.Week;

@cryptiklemur
Copy link
Author

cryptiklemur commented Aug 19, 2016

Ah, well that is disappointing

@JonPSmith
Copy link

JonPSmith commented Nov 9, 2016

Yes, disappointing as no warning that this would happen.

I would have preferred a NotImplementedException rather than silently failing

@aloisdg
Copy link
Contributor

aloisdg commented Nov 9, 2016

Indeed I think it should be loud too. Better, it should be implemented! 😉

@JonPSmith Could you look into it? Thank you ☺

@JonPSmith
Copy link

Hi @aloisdg,

I have participated in a few projects, but I'm on two contracts at the moment so I don't have the time at the moment.

@aloisdg
Copy link
Contributor

aloisdg commented Nov 10, 2016

@JonPSmith No problem!

@redwards510
Copy link

Here's a simple test case to illustrate this bug.
var result_is_6days = TimeSpan.FromDays(370).Humanize(1, null, TimeUnit.Month, TimeUnit.Second, ", ");

Apparently it breaks going from TimeUnit.Week to TimeUnit.Month

@reinhardtb
Copy link

Hi -

I would like to contribute the fix for this issue to the project, though I can't seem to complete a pull request back into the dev branch. Would someone please get into touch with me to get this going (sorry, it's my first time :) )?

Thanks,
Reinhardt

@hangy
Copy link
Contributor

hangy commented Dec 12, 2016

I think your PR #604 could be fine. Just check the failing unit tests. It appears that they currently validate that the cases you want to fix do not run, so it's probably just a matter of correcting the current unit tests and expanding them to check for the newly supported changes.

@reinhardtb
Copy link

Cool - thanks for the feedback - will check out those unit tests!

@akamud
Copy link
Contributor

akamud commented Feb 12, 2017

Before we start writing any code, we should see how we should convert days to months/years.

A month has 28 days (4 weeks) or 30 days?
A year has 365 or 366 days?

Or should we use the exact value (1 month = 30.436875 days) and round it?

@clairernovotny
Copy link
Member

The challenge with anything date related is that the year and locale often matter. You can use the TimeZoneInfo class to get the rules for how many days a particular year has (leap year) or however many days a month has (per month). But it's hard to do that w/o context.

Not sure the right answer here.

@JonPSmith
Copy link

I had a come up with a solution for the project I was on. I found it was really hard to do when staying in TimeSpan but I made it work for DateTime.

I found the various answers to the stackeoverflow question Date difference in years using C# useful. In the end worked out the years using dana's answer.

I hope that helps.

@hazzik
Copy link
Member

hazzik commented Feb 12, 2017

Bare TimeSpan would not make any sense without reference to a fixed point in time. E.g. 366 days is just 366 days.

But 366 days ago can be 1 year or 1 year and a day ago depending on a context.

We need to make sure that we distinguish between bare TimeSpan and TimeSpan as a difference of 2 dates & times.

For a bare TimeSpan we could implement a fuzzy logic, saying that 366 days is about 1 year ago.

@akamud
Copy link
Contributor

akamud commented Feb 12, 2017

I like the idea of using fuzzy logic for bare TimeSpan

@aloisdg
Copy link
Contributor

aloisdg commented Feb 13, 2017

I think the fuzzy logic is good enough. It could be more precise, but I guess thats fine.

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

No branches or pull requests

9 participants