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

[feature] Add support for k and kk format parsing #3766

Closed
wants to merge 1 commit into from

Conversation

EzequielB
Copy link
Contributor

k and kk parsing support behaves the same as H and HH, except for the value 24 that is treated as the 0 value for the HOUR field.
I had a concern about how to treat the 25 value, since moment("24:00", "HH:mm") is valid but moment("24:50", "HH:mm") is not; moment("25:00", "kk:mm") could be valid (setting the hour field to 1), but in the end that seemed counterintuitive.

Any thoughts are welcomed.
Hope this helps.

@ichernev
Copy link
Contributor

@EzequielB change looks correct. Is this k token used in some other time library?

@EzequielB
Copy link
Contributor Author

@ichernev, yes java's SimpleDateFormat uses it for example (http://docs.oracle.com/javase/6/docs/api/java/text/SimpleDateFormat.html), and momentjs is also using it for formatting currently (not parsing).
(Just to mention my use case, I was willing to parse dates from image file names that Android vanilla cam generates, that uses k format for the hours).

@ichernev ichernev added this to the 2.18.0 milestone Mar 5, 2017
@ichernev
Copy link
Contributor

ichernev commented Mar 5, 2017

OK, makes sense. Can you please make a PR to our docs repo: https://github.com/moment/momentjs.com

@westy92
Copy link

westy92 commented Mar 6, 2017

Does the TypeScript definition file need to be updated as well?

@EzequielB
Copy link
Contributor Author

@ichernev , docs were updated (PR moment/momentjs.com#400)
Let me know if all is ok.

@ichernev
Copy link
Contributor

@westy92 I don't think so. Parse tokens are not part of the spec.

@ichernev
Copy link
Contributor

Merged in 32438b4

@ichernev ichernev changed the title Add support for k and kk format parsing [feature] Add support for k and kk format parsing Mar 11, 2017
@ichernev ichernev closed this Mar 11, 2017
ichernev added a commit that referenced this pull request Mar 11, 2017
[feature] Add support for k and kk format parsing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants