Revert PR#376. #559
Revert PR#376. #559
Conversation
The final form of these files as merged into main is not at all what was intended in the original PR, and the tests are not correct: originally, they were tests taken from later drafts of keywords that were introduced in these later drafts, but with all the results changed to 'true' in order to demonstrate that the keywords were NOT recognized - that they were entirely ignored. The tests were moved to optional/, apparently with the intent to express that an implementation might wish to recognize those keywords anyway (which seems odd, but could be worth a discussion to see if this would be useful), but the results were never changed from being all 'true' back to the actual true-or-false results as in the specification version where they were introduced. Consequently they are not valid tests and should be removed.
|
This is incorrect, the tests are correct as merged. Please read the comment I left on the PR, and feel free to respond to it. |
|
The above explanation is elaborated on in #376 (comment). Closing, but follow up if this still isn't clear enough. |
|
If they stay, the duplicate $id in the draft7 tests should be fixed. This is apparent as soon as anyone tries to run them. |
|
Also -- these tests for draft2019-09 and later aren't optional if/when we put in an explicit $schema keyword. In those drafts, the tests are saying "we expect you to be following this exact dialect, with nothing extra", because we have a way of expressing that now. |
Great, let's fix that.
Please read my comments. That's not correct, at least not to my reading of the spec. If you disagree, cite where it says so. I noted that here. |
|
@karenetheridge I do think that @Julian is correct that extra keywords are still allowed and ignored, and the part of the spec he quoted indicates that. |
|
Merging, and will send a new PR reverting the merge so we can discuss after these are backed out, if that's what'd be helpful. |
The intent of them is *not* to express that an implementation might wish to recognize these keywords. The intent of them is precisely the same as the original intent -- to allow an implementation to test itself for *not* recognizing new keywords. Such an implementation should enable these tests, as it is free to do so, and these tests will help ensure it does not accidentally leak newer keywords backwards in time if it doesn't mean to. All of the previous discussion was strictly centered on not putting these in the required directory. The reason is that all drafts *allow* implementations to add extra keywords. So a compliant implementation may indeed decide it actively *wants* to implement new keywords. Then this file would not help them, and they wouldn't run it. They would presumably love a version of this file that *did* contain all the future assertions as correct for future drafts, and in the future we could provide one. But given no implementations really do do this, at least as far as I'm aware, and given that this version of the file is here and written and more common, it would seem perfectly reasonable to have it and hope for the other version if or when someone wants it. Further discussion is in #383 and later #559. This reverts commit f605fbf.
|
Let's discuss on #561 so we're not using a closed PR. |
The final form of these files as merged into main is not at all what was
intended in the original PR, and the tests are not correct: originally, they
were tests taken from later drafts of keywords that were introduced in these
later drafts, but with all the results changed to 'true' in order to
demonstrate that the keywords were NOT recognized - that they were entirely
ignored. The tests were moved to optional/, apparently with the intent to
express that an implementation might wish to recognize those keywords anyway
(which seems odd, but could be worth a discussion to see if this would be
useful), but the results were never changed from being all 'true' back to the
actual true-or-false results as in the specification version where they were
introduced. Consequently they are not valid tests and should be removed.