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

Fix parsing AM/PM hours #9334

Merged
merged 9 commits into from
May 28, 2020

Conversation

straight-shoota
Copy link
Member

Fixes #9333

@jhass
Copy link
Member

jhass commented May 21, 2020

Not quite there yet.

bin/crystal eval 'p Time.parse("12", "%H", Time::Location::UTC)'
Using compiled compiler at .build/crystal
0001-01-01 00:00:00.0 UTC

I think we need to turn @pm into a tri-state of "AM", "PM", "24 hour clock"

@straight-shoota
Copy link
Member Author

So we're missing even more specs... =)

@jhass
Copy link
Member

jhass commented May 21, 2020

Might also want to add some to verify %l and %I on its own without %P parses to am.

src/time/format/parser.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member Author

I found another issue and fixed it in 9a90d33: On the 24-hour clock, 24:00 is a valid time indicating midnight and equivalent to 00:00 of the next day (see ISO 8601 §5.3). The validation in Time#initializer incorrectly rejected this.

@jhass
Copy link
Member

jhass commented May 21, 2020

bin/crystal eval 'p Time.parse("12", "%l", Time::Location::UTC)'
Using compiled compiler at .build/crystal
0001-01-01 12:00:00.0 UTC

Should this be 00:00?

If yes, make sure %P%l still parses PM12 to 12:00.

@straight-shoota
Copy link
Member Author

Hm, I guess either could be right?

Ruby parses 12 with %I as 12. libc's strptime parses it as 0.

Maybe Ruby's implementation didn't think about this case. And it's really rare.
I have not preference which one it should be.

@jhass
Copy link
Member

jhass commented May 21, 2020

Well, for 0-11 it parses as AM, so why not parse the full range as AM?

@straight-shoota
Copy link
Member Author

Okay, I'll use that. That means Time.parse("00", "%I") should be invalid, at least it's in both Python and Ruby (which seems to indicate that Ruby's behaviour might not be fully intentional).

@straight-shoota
Copy link
Member Author

Also added a spec for when 24-hour clock hour is used with am/pm (which is ignored).

@jhass
Copy link
Member

jhass commented May 22, 2020

Okay, I'll use that. That means Time.parse("00", "%I") should be invalid, at least it's in both Python and Ruby (which seems to indicate that Ruby's behaviour might not be fully intentional).

So right now this rejects 0 for %l but accepts 0am for %l%P, but the rationale was that if no %P is given for %l/%I, then we assume AM. And both Ruby and Python seem to follow that rationale (except for Ruby's 12AM bug) and reject 0AM generally.

I don't care too much which way we go, looking at https://en.wikipedia.org/wiki/12-hour_clock#Confusion_at_noon_and_midnight 0AM seems to be used by the Japanese at least sometimes, so I'd slightly favor allowing it. But then even Ruby does not parse it (being of Japanese origin). In any case 0 and 0AM should parse the same in all cases for %l and %l%P respectively.

@straight-shoota
Copy link
Member Author

Oh yeah 0 am should be invalid, too. I'm not sure why I added it...

src/time/format/parser.cr Outdated Show resolved Hide resolved
Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to implement the most common interpretation according to https://en.wikipedia.org/wiki/12-hour_clock#Confusion_at_noon_and_midnight now

@straight-shoota straight-shoota added this to the 0.35.0 milestone May 27, 2020
@bcardiff bcardiff merged commit e43364f into crystal-lang:master May 28, 2020
@straight-shoota straight-shoota deleted the fix/time-parse-am_pm branch May 28, 2020 17:18
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.

Time.parse doesn't work for 12 hour times between noon and 1pm
3 participants