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

Add k and kk formatting tokens #3102

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
3 participants
@not-an-aardvark
Contributor

not-an-aardvark commented Apr 10, 2016

This fixes issue #2762.

The consensus from that issue seemed to be that the presence of the kk token should not modify the value of other tokens, so that is the behavior that is implemented here.

It should be noted that this decision introduces a bit of an inconsistency, given that the parsing behavior introduced here does change the day when an hour of 24 is present.

In other words,

moment('2016-04-09T24:00:00')
// yields a moment representing midnight at the start of April 10th, introduced in #1965

moment('2016-04-09T24:00:00').format('YYYY-MM-DD[T]kk:mm:ss')
// "2016-04-10T24:00:00" (different from the original format string that was parsed)

I'm not extremely familiar with the internal codebase, so please let me know if there's anything that I did incorrectly or that I should add.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 12, 2016

Looks good, thank you @not-an-aardvark ! Will merge in next release.

@ichernev ichernev added this to the 2.13.0 milestone Apr 12, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 16, 2016

Merged in 486a9db

@ichernev ichernev closed this Apr 16, 2016

ichernev added a commit that referenced this pull request Apr 16, 2016

@not-an-aardvark not-an-aardvark deleted the not-an-aardvark:kk-format-token branch Jul 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment