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

Added colon separated time support #102

Closed
wants to merge 1 commit into from

Conversation

barinali
Copy link

This branch covers #24.

I don't know why @stuartpb closed his pull request, but I wanted to mention him anyway. Because I just refactored his code block a little bit and added some tests to cover the feature.

@stuartpb
Copy link

stuartpb commented Nov 1, 2017

Worth noting that the closed pull request in question was #25, which was written to close #24

@stuartpb
Copy link

stuartpb commented Nov 1, 2017

Okay, this was four years ago, so it's a bit of a challenge for me to remember back to when I wrote the PR, but if I remember correctly, the reason I closed it was because, on testing, the parsing failed because ms.js didn't actually handle mixed times (ie. it could handle 3h or 4m but not 3h4m). It's possible that's changed since the original PR, but from what I remember, that was the failure I experienced with the code submitted in #25.

@stuartpb
Copy link

stuartpb commented Nov 1, 2017

Yeah, I'm pretty sure that's what it was - and, for that reason, this PR is busted, too: you're returning a rewritten string when given the parse input with a colon instead of a value in milliseconds (a rewritten string that, as the original issue still appears to be present in ms.js's implementation, would not parse to the correct time if you were to pass it back in to ms()).

@stuartpb
Copy link

stuartpb commented Nov 1, 2017

The issue in question was raised in #54. A solution was proposed with #55 but never merged.

@rauchg
Copy link
Member

rauchg commented Dec 3, 2018

Not a fan of this. Seems out of scope for ms

@rauchg rauchg closed this Dec 3, 2018
@rauchg
Copy link
Member

rauchg commented Dec 3, 2018

Thank for the proposal!

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

Successfully merging this pull request may close these issues.

3 participants