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

Change DefaultDateTimeHumanizeStrategy to turn 60 min to one hour not 45 #238

Closed
MehdiK opened this issue Apr 21, 2014 · 5 comments
Closed
Milestone

Comments

@MehdiK
Copy link
Member

MehdiK commented Apr 21, 2014

DefaultDateTimeHumanizeStrategy has an inconsistent behavior: it returns 59 seconds for 59 seconds but returns one hour for 45 minutes!! If someone wants 45 minutes to report an hour, they would most likely want 45 seconds to return a minute too, in which case they can use the PrecisionDateTimeHumanizeStrategy that does this.

@MehdiK MehdiK added the jump in label Apr 28, 2014
@MehdiK MehdiK added this to the V2 milestone Apr 28, 2014
@preetksingh80
Copy link
Contributor

hey, MehdiK,

I have picked up this issue, I was just looking at the test and in

DateHumanizeDefaultStrategyTests
, the test below says that when you pass 45 minutes, it should return "and hour ago"

[Theory]
        //[InlineData(1, "a minute ago")]
        //[InlineData(10, "10 minutes ago")]
        //[InlineData(44, "44 minutes ago")]
        [InlineData(45, "an hour ago")]
        //[InlineData(119, "an hour ago")]
        //[InlineData(120, "2 hours ago")]
        public void MinutesAgo(int minutes, string expected)
        {
            DateHumanize.Verify(expected, minutes, TimeUnit.Minute, Tense.Past);
        }

Is this the desired behavior. or is the test wrong as well. Please let me know, assuming the test is wrong , i have fixed the both and will ask for a pull request soon.
Many thanks
Preet

@MehdiK
Copy link
Member Author

MehdiK commented May 23, 2014

Hey @preetksingh80,

Cool. Yup. Change away all tests that fail because of this change. Please fetch the latest and rebase before sending the PR as another PR just got merged in. Also check out the contribution guideline for other points.

@preetksingh80
Copy link
Contributor

thanks mate, I am a bit new to github :(, sorry. please you will have to bare with me. also I have fixed it but please can you check if the behavior below would be correct, if so the tests are passing now!

 [InlineData(1, "a minute ago")]
        [InlineData(10, "10 minutes ago")]
        [InlineData(44, "44 minutes ago")]
        [InlineData(45, "45 minutes ago")]
        [InlineData(59, "59 minutes ago")]
        [InlineData(60, "an hour ago")]
        [InlineData(119, "an hour ago")]
        [InlineData(120, "2 hours ago")]

@MehdiK
Copy link
Member Author

MehdiK commented May 23, 2014

No need for apology. That's all good. Push it up :)

@MehdiK
Copy link
Member Author

MehdiK commented Jun 13, 2014

This was fixed in #278

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

3 participants