Skip to content
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

unevaluated* updates (and some annotation dependency cleanup) #1541

Merged
merged 10 commits into from
Nov 7, 2024

Conversation

gregsdennis
Copy link
Member

What kind of change does this PR introduce?

Updates for spec release

Issue & Discussion References

Summary

Updates the unevaluated* keywords and sections that reference them (e.g. "this keyword's annotation affects the behavior of this other keyword").

Does this PR introduce a breaking change?

@gregsdennis gregsdennis changed the title unevaluated* updaets (and some annotation dependency cleanup) unevaluated* updates (and some annotation dependency cleanup) Oct 12, 2024
@gregsdennis gregsdennis self-assigned this Oct 12, 2024
jsonschema-core.md Outdated Show resolved Hide resolved
Copy link
Member

@jviotti jviotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much, much clearer. I love this improvement. I always found annoying that keywords like these were defined so much on top of annotations.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see this annotation dependency cleanup get done 🎉

jsonschema-core.md Outdated Show resolved Hide resolved
jsonschema-core.md Outdated Show resolved Hide resolved
gregsdennis and others added 2 commits October 15, 2024 10:51
Co-authored-by: Karen Etheridge <ether@cpan.org>
@gregsdennis
Copy link
Member Author

@karenetheridge please review the changes you requested.

Comment on lines 1657 to 1658
This keyword applies its subschema to all instance elements at indices greater
than the length of the `prefixItems` array in the same schema object. If
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This keyword applies its subschema to all instance elements at indices greater
than the length of the `prefixItems` array in the same schema object. If
This keyword applies its subschema to all instance elements at indices greater
than or equal to the length of the `prefixItems` array in the same schema object. If

Given that json arrays are indexed from 0: If prefixItems had 3 items, then the items keyword applies to all elements at index 3 and greater, not those greater than 3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

json arrays are indexed from 0

JSON 8259 doesn't specify and indexing base. Seems more like a programming language thing to me.

I'll see if I can fix this so that it doesn't mention indices at all.

Copy link

@mwadams mwadams Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

This keyword ignores instance elements in the array, up to and including the number of items found in the prefixItems array in the same schema object. It then applies its subschema to all subsequent instance elements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.rfc-editor.org/rfc/rfc6901.html#section-4 specifies that JSON pointer indexes are zero-based.

I don't see how we can avoid indexes because that's a necessary part of the annotation content for these keywords (and contains contains a list of indexes, not just the number of evaluated/validated elements), and we also use array indexes in any JSON pointer in a $ref, and in keywordLocations and absoluteKeywordLocations in error objects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good point. I think it's important to state that we're using zero-based indexing and reference that section.

I'll work that in. Thanks for the... pointer 😏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this in as-is (without references to indexing), but I've also added a section specifically about array indices.

@@ -1784,7 +1773,8 @@ keyword's annotation causes `contains` to assume a minimum value of 1.

The value of this keyword MUST be a valid JSON Schema.

This keyword applies its subschema to array elements.
This keyword applies to array instances by applying its subschema to the array's
elements.

An instance is valid against `contains` if the number of elements that are valid
against its subschema is with the inclusive range of the minimum and (if any)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be "... is within the ...", but doesn't relate to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. Can you elaborate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're commenting on a line I didn't change. Yeah, I think you're right. Will update.

jsonschema-core.md Show resolved Hide resolved
@gregsdennis gregsdennis requested a review from mwadams October 26, 2024 02:08
@gregsdennis
Copy link
Member Author

@karenetheridge please review changes per your comments.

Copy link
Member

@karenetheridge karenetheridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great; thanks!

@gregsdennis gregsdennis merged commit 65460bb into main Nov 7, 2024
5 checks passed
@gregsdennis gregsdennis deleted the gregsdennis/unevaluated branch November 7, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants