Replaced ISO8601 with new code #2009
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2009 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 104 104
Lines 2203 2183 -20
Branches 477 468 -9
=========================================
- Hits 2203 2183 -20
Continue to review full report at Codecov.
|
|
@rubiin, can you please review this PR. |
Signed-off-by: Sahil Suman <sahilsuman933@gmail.com>
sahilsuman933
force-pushed
the
issue-2003
branch
from
ea83305 to
9948433
Compare
Signed-off-by: Sahil Suman <sahilsuman933@gmail.com>
There was a problem hiding this comment.
The original code was mostly based on http://www.pelagodesign.com/blog/2009/05/20/iso-8601-date-validation-that-doesnt-suck/
Is that article incorrect?
This PR also removes the options that we had so we'll have to deprecate those first and release this rewrite in the next major release or add some sort of backwards compatibility. Agree @rubiin ?
There was a problem hiding this comment.
While there seem to be real bugs in the code, as evidenced by the linked issues, you have not properly accounted for the removal of many of the valid ISO8691 timestamps in the test code.
If they are valid ISO8601 timestamps they should pass the test. If they are not valid and were added to the test code erroneously, that ought to be corrected, of course.
Why did you not add 2018-09-11T10:16.000Z (from #2003) to invalidISO8601? That is one of the key bugs your are trying to fix, so you should test for it.
2021-12-13T04:29:08+14:01 and 2021-12-13T04:29:08-12:01 (from #1883) should also be added to invalidISO8601.
| '2009-05', | ||
| '2009-123', | ||
| '2009-222', |
There was a problem hiding this comment.
This is a valid ordinal date.
| '2009-222', | ||
| '2009-001', | ||
| '2009-W01-1', | ||
| '2009-W51-1', |
There was a problem hiding this comment.
This is a valid Week with weekday
| '2009-W51-1', | ||
| '2009-W511', | ||
| '2009-W33', | ||
| '2009W511', |
There was a problem hiding this comment.
This is a valid week with weekday without separators
| '20090519', | ||
| '2009123', |
There was a problem hiding this comment.
These are valid ISO dates. Separators (hyphens) can be omitted
The ISO 8601 standard allows these separators to be omitted (e.g., 19980512 for a date), but expressions are much easier to read when separators are used.
| '2009-05-19 14:39:22-06:00', | ||
| '2009-05-19 14:39:22+0600', | ||
| '2009-05-19 14:39:22-01', | ||
| '20090621T0545Z', |
There was a problem hiding this comment.
Valid ISO8601 without separators
| '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', |
There was a problem hiding this comment.
I don't have the actual spec on hand, but the article @WikiRik linked includes these examples as valid, so I can only assume that this is some obscure syntax that is still a part of the spec. As the article says, If we are invoking the name ISO8601 we ought to respect the spec, even though it includes some wacky things.
|
Do I have to make any changes @braaar cause it's been 27 days and this PR is still not merged after getting the approval. |
I don't intend to cause offence here, but I think this PR is far from mergeable. I don't know what @rubiin's thoughts are, but are you can see from my comments there are a number of unexplained (possibly breaking) changes that I think should be accounted for before this is ready to merge. I would suggest you go back and answer my comments individually with your explanation for the changes. You should also address @WikiRik's comments about breaking the existing options and backwards compatibility. |
|
In order to fix this issue, I referred to Joi, validated all the test cases, and separated them according to their validation. Also, I thought it unnecessary to have a strict separator cause most of the validators don't have it, so I removed it, and the article that @WikiRik mentioned is quite old that's why I didn't follow that article. So what do you want me to do precisely @braaar? |
Joi isn't the arbiter of what test cases to include here, the International Organization for Standardization is. Does Joi implement the ISO8601 spec correctly?
If it passes the tests, bravo! You have shortened the code and fixed the bugs! If it doesn't, your code doesn't work properly and needs to be fixed. If the breaking changes were made purposefully, we need to have a conversation about which variants of the ISO8601 spec we might want to deprecate. You seem to want to deprecate a whole lot of variants of ISO860, but you have not actually argued this point so I am left guessing as to your intentions. What I think is the real solution I suspect that there is a divide between people who want to accept the entire ISO8601 spec, and people who want a stricter validator that accepts only a specific subset of the ISO8601 spec, such as I would be very happy to see some documentation about the JavaScript conventions for the ISO8601 spec. Have you found any such documentation? I may completely misread your intentions here. I am assuming you don't want |
|
I don't want to patronize here, but I think you have a narrow interpretation of ISO8601, since you wrote this in #2003
This is a valid ordinal date, at least according to the wikipedia article for ISO8601 (and the aforementioned article about ISO8601 validation) |
|
Yeah, you are right @braaar Joi isn't the arbiter. After this conversation with you, I came to know that I had a narrow interpretation of ISO8601. I used to think something like this format is a valid ISO8601 So, I will first learn about what ISO8601 is and how many divisions we can break this format to cover the large majority of demands. I will give you a summary before implementing this. This whole process might take some time but don't worry, I will not leave this PR hanging until it is merged. :) |
|
I did my study on ISO8601, and I found out that it is unnecessary to cover the whole interpretation of ISO8601. Implementing it only for the Date in UTC format will be better as it is the most used format. Also, if you see, most of the issues that we got are related to this format only. So, I propose converting the entire validation algorithm for the Date in UTC format. Also, I will improve the implementation of the strict mode. What's your opinion about this, @braaar, @WikiRik and @rubiin? Should I proceed with the implementation? |
I think fixing the bugs on the existing validator should be a priority. A new strict mode with a more narrow conception of ISO8601 will probably be appreciated by many. Feel free to drop a comment here explaining exactly which format(s) you intend to allow in the strict mode. |
|
Now, I feel like we don't even need the strict mode. But, still, if we want to add it, we can cover the ordinal date, week with weekends and week. // @braaar |
I'm starting to think that maybe it's best to have one PR for fixing the bugs you linked here, and one PR for new features (and possibly breaking changes). It could also make more sense to create an issue where people can discuss what options they would like to have. You can propose modes such as
In that issue we could also discuss what we want to do in terms of breaking changes. If we want for example the UTC one to be the default, we should schedule it as a breaking change for the next major release. Obviously we have to agree first on what is the best thing to have as the default behaviour, however. |
|
Okay, @braaar. I will close this PR and create a new one to fix those issues. Also, can you tell me where I can discuss these new features? |
Create an issue? :) |
Signed-off-by: Sahil Suman sahilsuman933@gmail.com
Fixes - #2003 #1883 #1046
Checklist