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.

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

fix datetime format for RFC3339 compliance #82

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix datetime format for RFC3339 compliance #82

wants to merge 7 commits into from

Conversation

MarcoKorinth
Copy link

@MarcoKorinth MarcoKorinth commented May 17, 2023 •
edited
Loading

Fixes #55
Does work for fast mode and full mode.

Fix tested with this example:
ajv.validate({ type: 'string', format: 'date-time' }, '2020-01-01 20:00:00.000Z')

@tschmidtb51 tschmidtb51 mentioned this pull request Jul 11, 2023
Open
tschmidtb51
tschmidtb51 suggested changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

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

I still found some cases not covert yet...

src/formats.ts Outdated Show resolved Hide resolved
tests/index.spec.ts Outdated Show resolved Hide resolved
tests/index.spec.ts Show resolved Hide resolved
@MarcoKorinth MarcoKorinth requested a review from tschmidtb51 July 13, 2023 12:19
tschmidtb51
tschmidtb51 reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

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

One minor comment - other than that: LGTM

tests/index.spec.ts Outdated Show resolved Hide resolved
tschmidtb51
tschmidtb51 suggested changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

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

Please recheck that the line 200 change does not break the iso-date-time.

src/formats.ts Show resolved Hide resolved
@MarcoKorinth MarcoKorinth requested a review from tschmidtb51 July 13, 2023 22:08
tschmidtb51
tschmidtb51 suggested changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

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

I found a duplicate and a mistake in the testcases.

tests/extras/format.json Outdated Show resolved Hide resolved
tests/extras/format.json Outdated Show resolved Hide resolved
tschmidtb51
tschmidtb51 approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

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

LGTM

@tschmidtb51
Copy link
Contributor

@epoberezkin, @ChALkeR Please review

@tschmidtb51
Copy link
Contributor

@ChALkeR , @epoberezkin Please review the suggested changes

@tschmidtb51
Copy link
Contributor

@jasoniangreen Could you please have a look an review this?

@tschmidtb51
Copy link
Contributor

@MarcoKorinth Please rebase.

@tschmidtb51
Copy link
Contributor

@lmammino, @jasoniangreen Please review.

@tschmidtb51
Copy link
Contributor

tschmidtb51 commented Jul 17, 2024 •
edited
Loading

@MarcoKorinth #86 points out that the leap second is implemented incorrectly. This might need some additional logic...

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

@tschmidtb51 tschmidtb51 tschmidtb51 approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

date-time accepts timestamps without "T" separator between date and time
2 participants
@MarcoKorinth @tschmidtb51