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

Move tests with ids in non-schemas to optional #707

Merged

Conversation

jdesrosiers
Copy link
Member

Resolves #700

This moves any tests that reference into locations not known to be a schema to the "optional" folder.

@jdesrosiers jdesrosiers requested a review from a team as a code owner November 20, 2023 20:10
@jdesrosiers jdesrosiers force-pushed the remove-non-schema-id-tests branch from 5677992 to b50e30b Compare November 20, 2023 20:14
@karenetheridge
Copy link
Member

I don't understand why this is being done -- are you expecting that implementations could either pass or fail these tests? But some of the things being tested here are mandatory -- such as that keywords that look like identifiers in non-schemas MUST NOT be treated as real identifiers. That's different from simply $ref-ing into non-schemas (which has undefined behaviour in the spec).

@gregsdennis
Copy link
Member

@karenetheridge would you then expect only the tests which have pointer $refs that go into non-schema locations be moved to optional? That would leave any tests which use $id-based $refs in required.

@karenetheridge
Copy link
Member

Yes, and I thought we'd already done that - because optional/refOfUnknownKeyword.json already exists.

Copy link
Member

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

Yeah, I agree with @karenetheridge. These tests definitely are required. I've also left a few comments on them that might not be related to this PR specifically.

(I supposed I should look more closely at the content of the PR than just check to see if my implementation passes.)

@jdesrosiers
Copy link
Member Author

Let's keep discussion in #700. @karenetheridge and @gregsdennis, I'd like to hear what you think about my last comment.

Copy link
Member

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

I'm good with these now. Thank you.

@jdesrosiers jdesrosiers merged commit 7a3d06d into json-schema-org:main Dec 5, 2023
2 checks passed
@jdesrosiers jdesrosiers deleted the remove-non-schema-id-tests branch December 5, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should identifier declarations be respected in non-schema locations?
3 participants