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: extra validation for dates #910

Merged
merged 2 commits into from
Oct 15, 2018

Conversation

profnandaa
Copy link
Member

This commit adds an additional test for dates
over and above passing the pattern match.

It checks to assertain that the date is a
valid date. For examples, dates like
2009-02-29 are caught as invalid.

Additionally, it also fixes a minor bug
with the weekDate that was wrongfully
leaving out W53 as an invalid date.

fixes #772

This commit adds an additional test for dates
over and above passing the pattern match.

It checks to assertain that the date is a
valid date. For examples, dates like
2009-02-29 are caught as invalid.

Additionally, it also fixes a minor bug
with the `weekDate` that was wrongfully
leaving out W53 as an invalid date.

fixes validatorjs#772
@profnandaa
Copy link
Member Author

@chriso - I hope this won't cause any major backward incompatibilities. I don't see the reason why someone would like to have something like 2009-02-29 pass as a valid date, as much as it passes for the pattern.

However, I've added an option to turn this off instead with options.strict = false if it will ever be needed. If you feel this is not a necessity, I can remove the options altogether.

Let me know.

@@ -5513,7 +5513,7 @@ describe('Validators', () => {
'2009123',
'2009-05',
'2009-123',
'2009-222',
// '2009-222', // invalid test-case, implies 2009-22-2
Copy link
Member Author

Choose a reason for hiding this comment

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

@chriso - I also found 2 invalid test cases in the cause of raising this PR, which I commented out. Let me know if this looks okay with you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a valid ordinal date. The 222 indicates that it's 222 days into the year.

Copy link
Collaborator

@chriso chriso left a comment

Choose a reason for hiding this comment

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

Happy to accept this one, but I'd make strict = false by default so that it's backwards compatible.

We had a date validator in the past that did something similar (#418, #431) but it was a source of bugs and confusion. The ISO 8601 validator is a lot more strict in the types of dates it accepts, however the standard still defines many different types of dates and so we may inadvertently make it too strict.

@profnandaa
Copy link
Member Author

Oops, I see, I did not take care of ordinal dates. Thanks for the catch! Let me refactor.

@profnandaa
Copy link
Member Author

@chriso - done, let me know. I've also included the previous tests when strict = false as regression to make sure it's backward compatible.

- adds provision for ordinal dates
- checks that ordinal dates are also valid for leap years
- includes regression tests for previous cases with `strict = false`
@profnandaa profnandaa merged commit dad8961 into validatorjs:master Oct 15, 2018
@profnandaa profnandaa deleted the fix-772-iso8601 branch October 15, 2018 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isISO8601 allows non-existent dates
2 participants