Skip to content

fix for {12,0} {am,pm} cases #26

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jul 19, 2017
Merged

fix for {12,0} {am,pm} cases #26

merged 8 commits into from
Jul 19, 2017

Conversation

safareli
Copy link
Contributor

fix #25

@safareli safareli requested review from cryogenian and garyb July 14, 2017 19:53
@safareli safareli mentioned this pull request Jul 14, 2017
Open
cryogenian
cryogenian approved these changes Jul 15, 2017
View reviewed changes
Copy link
Member

@cryogenian cryogenian left a comment

Choose a reason for hiding this comment

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

👍

@safareli safareli mentioned this pull request Jul 16, 2017
Closed
@safareli safareli changed the title fix for {P12,0} {am,pm} cases fix for {12,0} {am,pm} cases Jul 17, 2017
@safareli
Copy link
Contributor Author

safareli commented Jul 17, 2017 •
edited
Loading

It needs to fail on: 24:1:40

garyb
garyb reviewed Jul 18, 2017
View reviewed changes
test/src/DateTime.purs Outdated
let err = (Left "When hour is 24, other time component must be 0@1:24")
shouldEqual (FDT.unformatDateTime "YYYY-DD-MM HH:mm:ss:SSS" "0000-01-01 24:00:00:001") err
shouldEqual (FDT.unformatDateTime "YYYY-DD-MM HH:mm:ss:SSS" "0000-01-01 24:00:01:000") err
shouldEqual (FDT.unformatDateTime "YYYY-DD-MM HH:mm:ss:SSS" "0000-01-01 24:01:00:000") err
Copy link
Member

Choose a reason for hiding this comment

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

How do these pass? Is the "@1:24" in the error some magic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's represents position https://github.com/slamdata/purescript-formatters/blob/master/src/Data/Formatter/Parser/Utils.purs#L24

Copy link
Member

Choose a reason for hiding this comment

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

Can we print it as "When hour is 24, other time component must be 0 (line 1, col 24)" or something like that instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@safareli safareli merged commit 1097e73 into purescript-contrib:master Jul 19, 2017
@safareli safareli deleted the ampm branch July 19, 2017 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyb garyb garyb approved these changes

@cryogenian cryogenian cryogenian approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

format unformat of {12,0} {am,pm}, 24
3 participants
@safareli @garyb @cryogenian