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

Leap year and general date validation for isDate(). #431

Merged
merged 6 commits into from
Oct 5, 2015

Conversation

ravestack
Copy link
Contributor

This is a follow up from #418. The one gotcha is that it doesn't accept null as valid which the Date function does. However, this works just fine when I test the function by itself; it may just be an artifact of the testing suite.

Let me know if you'd like to see more tests.

@ravestack ravestack force-pushed the suchchange branch 2 times, most recently from 07fcb25 to 31fbd30 Compare August 31, 2015 01:31
@ravestack ravestack changed the title ignore colons in date strings for matching, more tests Leap year and general date validation for isDate(). Aug 31, 2015
@chriso
Copy link
Collaborator

chriso commented Sep 3, 2015

I'm a little bit wary of merging this considering how lenient Date.parse() is and how few tests there currently are. More tests would definitely be appreciated. The ISO8601 tests are a good start: https://github.com/chriso/validator.js/blob/b20a8db53b112eea86dfed9b9d6efabb4cde63de/test/validators.js#L2134

@ravestack
Copy link
Contributor Author

@chriso Just thought I'd let you know that the ISO 8601 dates in the test suite mismatch in some places with the JavaScript definition of valid date.

For example, these are all valid ISO 8601 dates but invalid JavaScript dates:

'2009123'
'2009-123'
'2009-222'
'2009-W01-1'
'2009-W51-1'
'2009-W511'
'2009-W33'
'2009W511'
'2009-05-19 14'
'2009-W21-2'
'2009-W21-2T01:22'
'2009-139'
'20090621T0545Z'
'2007-04-05T24:00'
'2010-02-18T16:23:48,444'
'2010-02-18T16:23:48,3-06:00'
'2010-02-18T16:23.4'
'2010-02-18T16:23,25'
'2010-02-18T16:23.33+0600'
'2010-02-18T16.23334444'
'2010-02-18T16,2283'
'2009-05-19 143922.500'
'2009-05-19 1439,55'

And these are valid JavaScript dates but invalid ISO 8601 dates:

'200905'
'2009-'
'2009-05-19 14:'
'200912-01'

I've updated my tests with ISO 8601 dates accordingly. I'm sure I need more, but I'd love suggestions as to what type of tests to add.

@chriso
Copy link
Collaborator

chriso commented Sep 28, 2015

Awesome, thanks. Going by these Date.parse() docs, some RFC 2282 tests might be helpful.

Not sure if this is a valid timezone offset or not, but isDate() isn't consistent here with Date.parse/new Date:

> validator.isDate('2008-01-01 00:00:00 GMT+0030')
false
> new Date('2008-01-01 00:00:00 GMT+0030')
Tue Jan 01 2008 08:30:00 GMT+0900 (KST)
> new Date(Date.parse('2008-01-01 00:00:00 GMT+0030'))
Tue Jan 01 2008 08:30:00 GMT+0900 (KST)

return false;
}
if (typeof str !== 'string') {
return true; //if Date.parse() passes, it's the current time
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't necessary. The first argument is always coerced to a string (1, 2, 3) based on these rules: https://github.com/chriso/validator.js#strings-only

@ravestack
Copy link
Contributor Author

I borrowed some RFC 2822 tests from around the web - I figured it'd be slightly better than trying myself - and I removed tests and checks for non-string types.

I also made sure to only check against actual double digit numbers, so that example you posted should pass.

@chriso
Copy link
Collaborator

chriso commented Oct 5, 2015

LGTM, thanks!

chriso added a commit that referenced this pull request Oct 5, 2015
Leap year and general date validation for isDate().
@chriso chriso merged commit eecb88f into validatorjs:master Oct 5, 2015
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.

2 participants