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

Allowing moment.set() to accept a hash of units #1809

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@jordansexton
Contributor

jordansexton commented Jul 30, 2014

Setting multiple values using the chaining API is awkward and verbose.

Currently:

moment().set('year', 2011).set('month', 9).set('date', 12).set('hours', 6).set('minutes', 7).set('seconds', 8);

After this patch, the current API is supported, as well as using a hash of values:

moment().set({ year: 2011, month: 9, date: 12, hours: 6, minutes: 7, seconds: 8 });

PR includes a passing test for the new behavior.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Aug 5, 2014

I think we need to enforce an order for setting the properties: from bigger units to smaller.

Also we need a few more tests that cover passing {} and null.

@ichernev ichernev added this to the 2.9 milestone Aug 5, 2014

@jordansexton

This comment has been minimized.

Contributor

jordansexton commented Aug 5, 2014

Got it. I'm not very familiar with moment's internals. What's the purpose of the ordering?
Also, I'm guessing it should noop on {} and return this. Should it also noop on null, should it error, or should it do something else (like a "clear" function)?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Aug 6, 2014

Nooping is fine. Ordering is for more consistent and less surprising results.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Dec 24, 2014

Merged in 81a3b7b

@ichernev ichernev closed this Dec 24, 2014

ichernev added a commit that referenced this pull request Dec 28, 2014

Merge pull request #1809 from jordansexton:develop
Allowing moment.set() to accept a hash of units
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment