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

Question: 11 days seems to output 1 week, is this the designed behaviour? #56

Closed
harouny opened this issue Jan 8, 2014 · 9 comments
Closed
Labels

Comments

@harouny
Copy link
Contributor

harouny commented Jan 8, 2014

[Fact]
public void ElevenDays()
{
    var elevenDays = TimeSpan.FromDays(11);
    var actual = elevenDays.Humanize();
    Assert.Equal("11 days", actual); //actual is (1 week) while I expect (11 days)
}

In my mindset I expect:
7 days -> 1 week
14 days -> 2 weeks
11 days -> 11 days

@MehdiK
Copy link
Member

MehdiK commented Jan 8, 2014

I think different people have different expectations from TimeSpan extension methods. There have been other confusions around this method too. See #29

To clarify the current implementation, the behavior somehow matches DateTime.Humanize where the bigger units override smaller units in the expense of accuracy; e.g. "23 hours ago" is "23 hours ago" while "32 hours ago" is reported as "yesterday". I hope it explains the mindset.

@harouny
Copy link
Contributor Author

harouny commented Jan 8, 2014

I see the problem but I think we must add an option to define the amount of data that we are OK to lose in favour of presentation. Otherwise I'm sure in a lot of cases it's not acceptable to lose this much of data.

@MehdiK
Copy link
Member

MehdiK commented Jan 8, 2014

I agree. That's being proposed in #29 and I will think about a way to do it. I also welcome a PR on this :)

I am interested to know why you think "11 days" should return "11 days" and not "1 week and 4 days" (or 1 week, 4 days)? 11 days in the timespan dialect equals 1.Week() + 4.Days(); so one could also expect (1.Weeks() + 4.Days()).Humanize() to return 1 week and 4 days. Thoughts?

@harouny
Copy link
Contributor Author

harouny commented Jan 8, 2014

Well, to deal with this issue we first need to think about a TimeSpan object as just a TimeSpan we should ignore the fact that it can be composed using Humanize extensions.

Then we should ask ourselves how humans express this data when talking to each others which is very interesting becuase different people express this information in different ways. But we need to provide a solution that works just like what most people expect it to work. We need to know if most people say 1 week and 4 days or just 11 days

In my case and the people around me, we tend to use a single unit to express this kind of information instead of combining multiple units. I will give you an example:
My wife was talking to her mother yesterday on the phone and she told her that "We will visit my home country in 59 days". She didn't say 1 month and 29 days. 59 days was easier for this bit of information to be transferred.

@MehdiK
Copy link
Member

MehdiK commented Jan 9, 2014

I agree that the way the TimeSpan is composed is immaterial. That was an example to show that one might expect to get 1 week and 4 days if they pass in the result of a (1 week + 4 days) TimeSpan.

This observation could be made about many of the Humanizer's methods because most of them work in a very certain way with a very certain but loose language. As you said we should find out which variation is more common and implement that.

@hazzik
Copy link
Member

hazzik commented Jan 9, 2014

I think we need atleast two different methods:

  1. get time span as closest whole time unit as the current behavour.

    (1.Weeks() + 4.Days()).Humanize() => 2 weeks or 1 week

  2. get time span as exact representation as proposed in TimeSpan's should show the time breakdown #29. This method should accept additional parameter precision.

    (1.Weeks() + 4.Days() + 5.Hours()).ToWords(Precision.Hours) => 11 days and 5 hours

    (1.Weeks() + 4.Days() + 5.Hours()).ToWords(Precision.Days) => 11 days

    Or as alternative to 1. we need a method Round which will round timespan to closest whole time untit.

    (1.Weeks() + 4.Days() + 5.Hours()).Round().ToWords() => 2 weeks or 1 week

@MehdiK
Copy link
Member

MehdiK commented Jan 9, 2014

Thanks @hazzik. I was thinking about adding an optional Precision parameter to the existing Humanize method:

public static string Humanize(this TimeSpan input, Precision precision = <<Precision.Default>>)

This way the existing calls to Humanize will work the same as before with default precision, which returns the biggest available unit in the provided input. If one is then interested to get a result with higher precision they can choose from available precisions. I am still unsure about the desired precisions to include in the enum: it could be Days, Hours etc with an addition Default value or it could be something like OneUnit, TwoUnits',ThreeUnitsetc whereOneUnitbecomes the current behavior andTwoUnitsreturns11 days and 5 hours`. The latter has the added benefit that you can specify the precision without knowing what goes it.

@MehdiK
Copy link
Member

MehdiK commented Jan 23, 2014

This is partially addressed in #65. Will provide a way to provide the largest unit later.

@MehdiK
Copy link
Member

MehdiK commented Mar 4, 2015

This is now partially implemented by #383 and #388. Closing this.

@MehdiK MehdiK closed this as completed Mar 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants