Skip to content

fix datetime format for RFC3339 compliance #82

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

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

Conversation

MarcoKorinth
Copy link

@MarcoKorinth MarcoKorinth commented May 17, 2023

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
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...

@MarcoKorinth MarcoKorinth requested a review from tschmidtb51 July 13, 2023 12:19
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

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.

@MarcoKorinth MarcoKorinth requested a review from tschmidtb51 July 13, 2023 22:08
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.

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.

MarcoKorinth and others added 7 commits July 16, 2024 22:40
Co-authored-by: tschmidtb51 <65305130+tschmidtb51@users.noreply.github.com>
Co-authored-by: tschmidtb51 <65305130+tschmidtb51@users.noreply.github.com>
Co-authored-by: tschmidtb51 <65305130+tschmidtb51@users.noreply.github.com>
@tschmidtb51
Copy link
Contributor

@lmammino, @jasoniangreen Please review.

@tschmidtb51
Copy link
Contributor

tschmidtb51 commented Jul 17, 2024

@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
Labels
None yet
Development

Successfully merging this pull request may close these issues.

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