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 options.from #101

Closed
wants to merge 5 commits into from
Closed

Added options.from #101

wants to merge 5 commits into from

Conversation

yoavmmn
Copy link
Contributor

@yoavmmn yoavmmn commented Oct 25, 2017

WHATSUP

This feature was proposed and discussed in issue #100.

BEFORE

This was a common pattern in my and others code.

Date.now() + ms('2h')

AFTER

It's super simple now to get the milliseconds of given time.

ms('2h', { from: Date.now() });

@yoavmmn yoavmmn mentioned this pull request Oct 25, 2017
@yoavmmn yoavmmn changed the title added options.from Added options.from Nov 20, 2017
@TooTallNate
Copy link
Member

Thank you for the PR. I'm still not sure if I'm sold on the idea.

The AFTER code is, in fact, longer than the BEFORE version, and I think the before version is also more explicit. It's not super clear to me what the benefit of this change ends up being.

@yoavmmn
Copy link
Contributor Author

yoavmmn commented Nov 29, 2017

@TooTallNate The benefit isn't huge, all you get is a more readable code.
I can see why you aren't sold on the idea, it's not a super necessary feature or anything like that. just a feature it'll be nice to have.

@TooTallNate
Copy link
Member

Thank you for understanding @yoavmmn. I think we're going to pass on this one for now. Looking forward to future pull requests from you though!

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.

None yet

2 participants