Replaced ISO8601 with new code

[sahilsuman933]

Signed-off-by: Sahil Suman [email protected]

• Replaced the iso8601 method with new code
• Added new test cases

Fixes - #2003 #1883 #1046

Checklist

• [x] PR contains only changes related; no stray files, etc.
• [x] README updated (where applicable)
• [x] Tests written (where applicable)

[codecov[bot]]

Codecov Report

Merging #2009 (a68859c) into master (1bb14e8) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2009   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          104       104           
  Lines         2203      2183   -20     
  Branches       477       468    -9     
=========================================
- Hits          2203      2183   -20     

Impacted Files	Coverage Δ
src/lib/isISO8601.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1bb14e8...a68859c. Read the comment docs.

[sahilsuman933]

@rubiin, can you please review this PR.

[sahilsuman933]

@rubiin and @WikiRik You can do the PR review now.

[sahilsuman933]

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.

[braaar]

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.

[sahilsuman933]

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?

[braaar]

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.

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?

So what do you want me to do precisely @braaar?

1. Undo all your changes to the test code
2. Add the following cases to invalidIso8601

• 2018-09-11T10:16.000Z
• 2021-12-13T04:29:08+14:01
• 2021-12-13T04:29:08-12:01

3. See if your new code passes the tests

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 2022-08-23T15:47:31+00:00 and 2022-08-23T15:47:31Z. There is a real concern that when accepting the entirety of the ISO8601 spec, some false positives can sneak through. It's entirely possible for someone to accidentally type a valid ordinal date when in fact the intention is to type a standard date and time. Contributors in Joi have had a similar discussion. This division should be sorted out by having an option (or options) for strictness. I believe removing all options is throwing the baby out with the bathwater.

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 isISO8601 to accept the entire ISO8601 spec, but if your goal is merely to fix the issues you linked, then this whole discussion doesn't belong in this PR.

[braaar]

I don't want to patronize here, but I think you have a narrow interpretation of ISO8601, since you wrote this in #2003

For example, 2009-222 is not a valid ISO8601 date but is put in the validISO8601.

This is a valid ordinal date, at least according to the wikipedia article for ISO8601 (and the aforementioned article about ISO8601 validation)

[sahilsuman933]

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 1997-07-16T19:20:30+01:00. but that's not the case, it seems. Also, you are right about not deprecating variants of 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. :)

[sahilsuman933]

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?

[braaar]

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.

[sahilsuman933]

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

[braaar]

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

• mode that accepts only the UTC date time `2022-08-23T15:47:31Z
• mode that accepts only the standard 8601 date time with time zone 1997-07-16T19:20:30+01:00
• mode that accepts the ones above, plus ordinal dates
• some kind of advanced configuration object where you can pick and choose which formats you want to accept?

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.

[sahilsuman933]

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?

[braaar]

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? :)