-
-
Notifications
You must be signed in to change notification settings - Fork 7
Add support for draft-2019-09 #131
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
base: main
Are you sure you want to change the base?
Conversation
jdesrosiers
left a comment
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.
You're trying to do too much. The changes to these keywords from 2019-09 to 2020-12 were almost entirely renaming. You just need to make copies of the existing items and prefixItems handlers and make minor edits. If your handlers look significantly different than the 2020-12 versions of these keywords, you're on the wrong track.
One test isn't close to enough. Like, I said before, go through the existing tests, translate them to 2019-09, and remove any duplicates. There are six tests that use the prefixItems keyword not counting unevaluatedItems tests. I expect at least that many new tests.
ProTip: Always make an effort to match the code style used in the repo you're contributing to. Install the ESLint plugin in your editor to get code style hints as you work.
ProTip: Always run the test, lint, and type-check scripts before submitting a PR (see "Contributing" in the README). The automation is there to double check us because we all forget things sometimes, but it looks unprofessional when you submit something that isn't ready.
| let index = 0; | ||
| while (true) { | ||
| const itemNode = Instance.step(String(index), instance); | ||
| if (!itemNode) break; | ||
|
|
||
| outputs.push( | ||
| evaluateSchema( | ||
| /** @type {string} */ (items), | ||
| itemNode, | ||
| context | ||
| ) | ||
| ); | ||
| index++; | ||
| } |
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 should be a for..of loop using Instance.iter.
| outputs.push( | ||
| evaluateSchema( | ||
| /** @type {string} */ (items), | ||
| itemNode, | ||
| context | ||
| ) | ||
| ); |
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.
There's no good reason for this function can to span more than one line. While there are some exceptions, please avoid multi-line function calls in this repo.
| */ | ||
|
|
||
| /** @type NormalizationHandler */ | ||
| const itemsDraft04 = { |
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.
Let's stick with the naming convention used in all the other normalization handlers.
| const itemsDraft04 = { | |
| const itemsDraft04NormalizationHandler = { |
| // If items is not a tuple, additionalItems is ignored | ||
| const items = | ||
| /** @type {{ schema?: { items?: unknown } }} */ | ||
| (context).schema?.items; | ||
|
|
||
|
|
||
| if (!Array.isArray(items)) { | ||
| return outputs; | ||
| } | ||
|
|
||
| const startIndex = items.length; | ||
|
|
||
| // additionalItems: true → always valid | ||
| if (additionalItems === true) { | ||
| return outputs; | ||
| } | ||
|
|
||
| let index = startIndex; | ||
|
|
||
| while (true) { | ||
| const itemNode = Instance.step(String(index), instance); | ||
| if (!itemNode) break; | ||
|
|
||
| // additionalItems: false → validate against false schema | ||
| if (additionalItems === false) { | ||
| outputs.push( | ||
| evaluateSchema( | ||
| /** @type {string} */ ("false"), | ||
| itemNode, | ||
| context | ||
| ) | ||
| ); | ||
|
|
||
|
|
||
| } else { | ||
| // additionalItems is a schema | ||
| outputs.push( | ||
| evaluateSchema( | ||
| /** @type {string} */ (additionalItems), | ||
| itemNode, | ||
| context | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| index++; | ||
| } |
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.
The current items keyword is literally no more than a rename of additionalItems. This handler should look a lot more like the current items normalization handler.
| { | ||
| "description": "items (tuple validation) - draft 2019-09", | ||
| "compatibility": "<=2019", | ||
| "schema": { | ||
| "type": "array", | ||
| "items": [ | ||
| { "type": "number" } | ||
| ] | ||
| }, | ||
| "instance": ["foo"], | ||
| "errors": [ | ||
| { | ||
| "messageId": "type-message", | ||
| "messageParams": { | ||
| "expectedTypes": { "or": ["number"] } | ||
| }, | ||
| "instanceLocation": "#/0", | ||
| "schemaLocations": ["#/items/0/type"] | ||
| } | ||
| ] | ||
| } | ||
| , |
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 needs to be indented properly.
@jdesrosiers
What’s included
New normalization handlers
items-draft04.js— implements pre-2020 tuple validation semanticsadditionalItems-draft04.js— implements legacy handling for items beyond the tupleCorrect keyword registration
https://json-schema.org/keyword/draft-04/itemshttps://json-schema.org/keyword/draft-04/additionalItemsNew test coverage
compatibility: "<=2019"prefixItems/itemscases where behavior differsNo behavior changes for draft-2020-12 or newer drafts
Implementation details
items-draft04mirrors the behavior ofprefixItems:evaluateSchemaper array positionNormalizedOutput[](nonullvalues)additionalItems-draft04mirrors the draft-2020-12itemshandler:itemsis an array (tuple form)items.lengthtrue,false, and schema valuesBoth handlers are implemented as simple applicators.
Test status
All existing tests pass
New draft-2019-09 tests pass
Total: 200 / 200 tests passing
Notes
This implementation follows the guidance discussed in the issue:
Happy to adjust anything if you’d like a different structure or naming.