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

Removed mutation of momentjs instance, instead moment as injection #5

Merged
merged 1 commit into from
Mar 24, 2016

Conversation

psulek
Copy link
Collaborator

@psulek psulek commented Mar 24, 2016

  • momentjs is not anymore direct dependency, just dev dep, so now lib must be required like this:
var moment = require('moment');
var preciseDiff = require("moment-precise-range")(moment);
  • new option joinSeparator is introduced
  • updated tests to work with those changes

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.276% when pulling 80123fa on psulek:master into bfd387e on mtscout6:master.

@mtscout6
Copy link
Owner

I like where you took it. Do you feel comfortable releasing this as 1.0.0 on npm? I'll add you as a collaborator to this repo and the npm package.

@mtscout6 mtscout6 merged commit 1e3d6e9 into mtscout6:master Mar 24, 2016
@mtscout6
Copy link
Owner

No special release process. Just:

$ npm version major
$ git push
$ git push --tags
$ npm publish

@psulek
Copy link
Collaborator Author

psulek commented Mar 24, 2016

Sure i can. But i'm thinking about better options management, e.g. extend default options with user specific. What do you think?

@mtscout6
Copy link
Owner

Not sure I follow.

@psulek
Copy link
Collaborator Author

psulek commented Mar 24, 2016

e.g, now if you omit opts param, you will get "all" default options (hour, min, sec, ...) to true, but when you provide opts param with only one param (hour) you have only hour in ouput.

example:

console.log('result1: ', moment.preciseDiff(m1, m2));
console.log('result2: ', moment.preciseDiff(m1, m2, {second: true}));
console.log('result3: ', moment.preciseDiff(m1, m2, {second: false}));

returns:

result1:  15 hours 45 minutes a few seconds
result2:  a few seconds
result3:  

expected result for result3 should be:
result3: 15 hours 45 minutes

In 3rd expression moment.preciseDiff(m1, m2, {second: false}) i want to exclude seconds from output, but now it means include only seconds.

But this will be breaking change for existing users, so we can figure out some solution which returns same output like now for old users, but some switch should works in new way.

What do you think?

@mtscout6
Copy link
Owner

I like that, it's worth describing in a new issue and PR though.

@psulek
Copy link
Collaborator Author

psulek commented Mar 24, 2016

will do, but later :-)

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