-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat: add new isDate() validator #1270
feat: add new isDate() validator #1270
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution and welcome to the project! 🎉 See my comments below.
es/lib/isEmail.js
Outdated
@@ -1,8 +1,12 @@ | |||
function _slicedToArray(arr, i) { return _arrayWithHoles(arr) || _iterableToArrayLimit(arr, i) || _nonIterableRest(); } | |||
function _slicedToArray(arr, i) { return _arrayWithHoles(arr) || _iterableToArrayLimit(arr, i) || _unsupportedIterableToArray(arr, i) || _nonIterableRest(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file from the diff since it's unrelated to the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was auto-generated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove it before committing. This is because of the difference in the build system.
'2015-07-15T07:00:00+0000', | ||
], | ||
invalid: [ | ||
'', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try add the following test cases:
2020-02-30, // invalid date
2019-02-29, // non-leap year
2020-04-31, // invalid date
I presume all those will pass as true (or fail as false). We will need to handle these cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since:
> new Date('2020-02-31')
2020-03-02T00:00:00.000Z
> new Date('2020-04-31')
2020-05-01T00:00:00.000Z
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!
a simple validation would be
> new Date('2020-02-30').getUTCDate()
1
> new Date('2019-02-29').getUTCDate()
1
> new Date('2020-02-29').getUTCDate()
29
we can compare it with the date supplied
the problem here is with the date format some may give as YYYY-MM-DD or MM-DD-YYYY
we can do that! my bad
any suggestion?
any better way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd not thought of that way, I think I like it! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm looks like there are many edge cases to be considered in this case,
what if a user supplies
MM-DD-YYYY
YYYY-MM-DD
YY-MM-DD
MM-DD-YY
we need to keep things simple and general
any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, for now let's just support those that are supported by Date()
, and strictly YYYY. Need to add that note on the README.
However, I noticed something else, look at this?
> new Date('02-02-2019')
2019-02-01T21:00:00.000Z // .getUTCDate() = 1
> new Date('2019-02-02')
2019-02-02T00:00:00.000Z // .getUTCDate() = 2
Perhaps this are some of the intricacies that led to deprecating the first isDate()
from the library. 🤔
What if we allowed the user to supply the format from a list of predefined formats?
MM-DD-YYYY
DD-MM-YYYY
YYYY-MM-DD
Then transform this to a YYYY-MM-DD
format before running it through new Date()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check out the fiddle
https://jsfiddle.net/hvrwp07e/1/
I have done some tweaking, I have some stuff to do so right now I don't have time to think about edge cases
feel free to introduce some edge case or enhance the code
let me know if you are aware of any edge cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to that we can do more addition on edge cases in the future and update the readme accordingly.
I will check on this @profnandaa. |
I have made the necessary changes to support almost all the format and handling leap years, invalid dates, etc I have added a few more test cases, let me know if we need more test cases have a look and let me know for any further changes! cc: @profnandaa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggesting a rework to simplify, let me know if it makes sense.
Thanks for the efforts so far.
return new Date(`${dateObj.m}/${dateObj.d}/${dateObj.y}`).getDate() === +dateObj.d; | ||
} | ||
|
||
return Object.prototype.toString.call(input) === '[object Date]' && isFinite(input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this last call for? The assumption is that input will always be a string. You need to call assertString()
at the top of the function...
@@ -0,0 +1,35 @@ | |||
function isValidFormat(format) { | |||
return /(^(y{4}|y{2})[\/-](m{1,2})[\/-](d{1,2})$)|(^(m{1,2})[\/-](d{1,2})[\/-]((y{4}|y{2})$))|(^(d{1,2})[\/-](m{1,2})[\/-]((y{4}|y{2})$))/gi.test(format); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd thought we'll come up with multiple regex depending with the format provided, starting with YYYY/MM/DD
as a default.
We said we'll keep it simple as just date string only, nothing else.
agreed that the regex part might seem complicated but its quite simple, I guess it's better to support all format with a little effort let me break down this code into simple chunks
function isValidFormat(format) {
return /(^(y{4}|y{2})[\/-](m{1,2})[\/-](d{1,2})$)|(^(m{1,2})[\/-](d{1,2})[\/-]((y{4}|y{2})$))|(^(d{1,2})[\/-](m{1,2})[\/-]((y{4}|y{2})$))/gi.test(format);
} this part checks for the following patterns
function zip(x, y) {
const zippedArr = [],
len = Math.min(x.length, y.length);
let i = 0;
for (; i < len; i++) {
zippedArr.push([x[i], y[i]]);
}
return zippedArr;
} the above one just zips two arrays into one, for example, consider the input as splitting the above inputs zipping them will yield this
after zipping the arrays in this part for (const [dateWord, formatWord] of dateAndFormat) {
if (dateWord.length !== formatWord.length) {
return false;
}
dateObj[formatWord.charAt(0)] = dateWord;
} we just check for length equality for eg.
then we init the date and check for validity by getting the date and the original input if both dates are same it is a valid date else nope let me know if anything is unclear, I hope this should work for almost all of the cases you can also refer the test cases cc: @profnandaa |
@mum-never-proud -- ok, sounds good. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ezkemboi -- ping |
Doing the review @profnandaa. I was held up by some work here. |
validator: 'isDate', | ||
valid: [ | ||
new Date(), | ||
new Date([2014, 2, 15]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we allow users to send created dates using the new Date()
method?
I thought, we just need to pass a string. But, it is a good idea.
src/lib/isDate.js
Outdated
return /(^(y{4}|y{2})[\/-](m{1,2})[\/-](d{1,2})$)|(^(m{1,2})[\/-](d{1,2})[\/-]((y{4}|y{2})$))|(^(d{1,2})[\/-](m{1,2})[\/-]((y{4}|y{2})$))/gi.test(format); | ||
} | ||
|
||
function zip(x, y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I would prefer if we make use of something like
function zip(date, format) {}
Use of x, y
a little bit makes the code harder to read.
src/lib/isDate.js
Outdated
len = Math.min(x.length, y.length); | ||
let i = 0; | ||
|
||
for (; i < len; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for(let i = 0; i < len; i++) {}
Might seem better than let i = 0
and use ;
to make separation.
@mum-never-proud -- ^ |
@profnandaa sorry was busy, I will update the changes in few hours! |
When is this releasing? |
Most likey by end of May or 1st half of June. |
Resolves #1130