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

Comments

Projects
None yet
5 participants
@rj33

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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.

@mj1856 mj1856 added the Bug label Jul 22, 2015

@ichernev

This comment has been minimized.

Contributor

ichernev commented Aug 11, 2015

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

@maggiepint

This comment has been minimized.

Member

maggiepint commented Oct 3, 2016

closed in favor of #3406

@maggiepint maggiepint closed this Oct 3, 2016

markstos added a commit to markstos/moment that referenced this issue Oct 20, 2016

Fix moment#2300: Default to current week.
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

Fix #2300: Default to current week.
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

Merge pull request #3515 from markstos:hotfix-issue-2300
[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