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

Adds 'mon' for Month #26

Merged
merged 2 commits into from
Nov 15, 2016
Merged

Adds 'mon' for Month #26

merged 2 commits into from
Nov 15, 2016

Conversation

IanMitchell
Copy link
Contributor

Added an abbreviation for month that I see most commonly in the app I'm working on

@coveralls
Copy link

coveralls commented Nov 14, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 1ef5c29 on IanMitchell:master into 213098f on mike182uk:master.

@mike182uk
Copy link
Owner

Hey @IanMitchell,

Thanks for this! Happy to merge this in, but could you make a few small changes:

  • Document the new keyword in the read me
  • Add a test for the new keyword (its fine to add a new assertion to it('can parse a timestring'...)

@coveralls
Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 5c7c064 on IanMitchell:master into 213098f on mike182uk:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5c7c064 on IanMitchell:master into 213098f on mike182uk:master.

@@ -15,6 +15,51 @@ describe('timestring', function() {
expect(timestring('1y')).to.equal(29030400);
});

it('can parse different unit identifiers', function() {
var unitMap = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to duplicate the map since it isn't exported, but I think violating DRY is ok here since looping over the unit types really reduces the amount of code. Happy to change how I wrote this test if you disagree though!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no problem, i like what you've done with the test, nice one 👍

@mike182uk
Copy link
Owner

@IanMitchell Thanks for this!

@mike182uk mike182uk merged commit dec42ce into mike182uk:master Nov 15, 2016
@mike182uk
Copy link
Owner

Released 3.2.0 which includes this change 👍

@IanMitchell
Copy link
Contributor Author

Thanks, much appreciated!

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