Add strict hour option for parsing 12-hour dates #1702
Conversation
|
This duplicates (and adds the requested |
diesieben07
left a comment
diesieben07
left a comment
There was a problem hiding this comment.
What happens if the users uses a macro token such as t and we get hourCycle: h11 from the options? Then we turn it into h, but that can't parse 0 (which is valid for h11).
…rmat-validation' into strict-mode-hour-format-validation
|
I've worked on this PR some more and implemented your requests. I noticed that there were no unit tests for invalid time DateTimes in general, so I've added tests that demonstrate the limits of what is valid and included (better) strictHours tests along with them. The actual 'strictHours' test logic I've implemented doesn't consider h11 or any other special situations, on account of my ignorance about those things. I'll look into that, but if you have any specific ideas about how you'd like to see the strictHours logic look, please let me know! update: I added another commit to handle the h11 strictHours case. |
diesieben07
left a comment
diesieben07
left a comment
There was a problem hiding this comment.
I really apologize for taking so long to get to this and thank you again for working on it.
I suggested some improvements.
| ? dateTimeFromMatches(matches) | ||
| : [null, null, undefined]; | ||
| if (strictHours && hasOwnProperty(matches, "h")) { | ||
| const isH11 = this.locale.getIntlLocaleHourCycles()[0] == "h11"; |
There was a problem hiding this comment.
Intl.Locale#getHourCycles is not supported in Firefox yet, so this would crash (the code is not checking if there is support).
A better option would be Intl.DateTimeFormat(locale, { hour: "numeric" }).resolvedOptions().hourCycle. Then you do not need all the extra code in locale.js and just need to add a new property (hourCycle) to Locale.
To add it, you can add { hour: "numeric" } to getCachedIntlResolvedOptions so that the resolved options will contain the hourCycle.
| accepts("0", "h", { strictHours: true, locale: "en-US-u-hc-h11" }); | ||
| rejects("0", "h", { strictHours: true }); | ||
| rejects("12", "h", { strictHours: true, locale: "en-US-u-hc-h11" }); | ||
| accepts("12", "h", { strictHours: true }); | ||
| rejects("13", "h", { strictHours: true }); |
There was a problem hiding this comment.
Can you add some tests for the other hour cycles as well?
| : [null, null, undefined]; | ||
| if (strictHours && hasOwnProperty(matches, "h")) { | ||
| const isH11 = this.locale.getIntlLocaleHourCycles()[0] == "h11"; | ||
| if (matches.h > (isH11 ? 11 : 12) || matches.h < (isH11 ? 0 : 1)) { |
There was a problem hiding this comment.
I don't think this is sufficient, there are 4 hour cycles (h11, h12, h23, h24). While MDN does state that h24 is not used anywhere (and it seems very strange) we should still honor it if it is set. Same goes for h12.
3 participants
Addressing #1625
and related
#1655
Adds a strict option to fromFormat which enables stricter parsing of 12-hour time formats, such that an error is thrown if the parsed hour is outside of the range 1-12.
By default strict parsing is disabled.
DateTime.fromFormat("18:30", "h:mm", { strictHours: true })will throwConflictingSpecificationErrorbecausestrictHoursis enabled.