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

dddd parsing and defaults uses start of year instead of something closer to current time #2300

Closed
rj33 opened this issue Mar 30, 2015 · 5 comments
Labels

Comments

@rj33
Copy link

rj33 commented Mar 30, 2015

var d = moment('Thursday 8:30pm', ['dddd h:mm a'], 'en');

var d2 = d.format("YYYY Do MMMM dddd h:mm a");

produces
2015 1st January Thursday 8:30 pm

Which I found surprising given the documentation for defaults. Is this intended behaviour?

@icambron
Copy link
Member

icambron commented Apr 1, 2015

Looks like Moment defaults to the first week if there's week data present: https://github.com/moment/moment/blob/develop/moment.js#L1401. I can see how this seems wrong, but I'm curious what a better behavior would be. The next Thursday after the current time? The Thursday in the current week? The latter would be relatively easy to implement by just changing that default.

@rj33
Copy link
Author

rj33 commented Apr 1, 2015

From reading the docs it seems defaults are supposed to be layered on top of current time by default, but as you say that still leaves the question as to whether it should be the day of week in the current week, or the day on which that day of week next falls. It feels to me like Thursday in the current week would be the closest match with the behaviour of the other defaults. I've worked around it in my own code for now by extracting the data I need out of the 1st January parse result and setting it explicitly on a new value with today's time.

@icambron
Copy link
Member

icambron commented Apr 1, 2015

I agree the docs are wrong (they're right for most cases, but Moment handles weekday/week/weekyear a bit separately from day/month/year). I think a pull request with appropriate tests that changes that default of 1 to the current week index would probably be accepted.

@ichernev
Copy link
Contributor

We should default to current week, as we default to current month/day. PRs welcome.

@maggiepint
Copy link
Member

closed in favor of #3406

markstos pushed a commit to markstos/moment that referenced this issue Oct 20, 2016
This update to Brian Schemp's patch calculates the current week once
instead of twice for a small performance boost.
ichernev pushed a commit that referenced this issue Nov 6, 2016
This update to Brian Schemp's patch calculates the current week once
instead of twice for a small performance boost.
ichernev added a commit that referenced this issue Nov 6, 2016
[feature] Fix #2300: Default to current week.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants