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 draft-specific files to the dedicated dir for its draft #753

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

karenetheridge
Copy link
Member

@karenetheridge karenetheridge commented Oct 7, 2024

..otherwise, draft-specific test suites won't know to skip adding the file

@karenetheridge karenetheridge requested a review from a team as a code owner October 7, 2024 21:01
@karenetheridge karenetheridge marked this pull request as draft October 8, 2024 17:10
@karenetheridge karenetheridge force-pushed the ether/fix-draft-locations branch 2 times, most recently from 8f5a987 to 96bc1ec Compare October 8, 2024 17:22
@karenetheridge karenetheridge changed the title move draft4-specific file to dedicated dir for this draft, and add $schema move draft-specific files to the dedicated dir for its draft Oct 8, 2024
@karenetheridge karenetheridge marked this pull request as ready for review October 8, 2024 17:23
@karenetheridge
Copy link
Member Author

Rebased, and also copied the draft6 and draft7 versions of this file.

All the remaining files in remotes/ that aren't in a draft-specific directory are compatible with draft2020-12, the latest published version. If we introduce other incompatibilties in the future we may need to move around some other files so they don't get used by the wrong versions.

@karenetheridge karenetheridge marked this pull request as draft October 8, 2024 17:26
..otherwise, draft-specific test suites won't know to skip adding the file
@karenetheridge karenetheridge force-pushed the ether/fix-draft-locations branch from 96bc1ec to a66d23d Compare October 8, 2024 17:41
@karenetheridge karenetheridge marked this pull request as ready for review October 8, 2024 18:01
Copy link
Contributor

@notEthan notEthan left a comment

Choose a reason for hiding this comment

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

Doesn't break anything for me

@karenetheridge karenetheridge merged commit 82a0774 into main Oct 9, 2024
4 checks passed
@karenetheridge karenetheridge deleted the ether/fix-draft-locations branch October 9, 2024 23:03
@Julian
Copy link
Member

Julian commented Jan 31, 2025

This is a disruptive PR with very little motivation from what can tell, inasmuch as it essentially leaves us in a very similar situation as before where it's very unclear which files are the ones that are meant to be read as part of each version's test run.

All the remaining files in remotes/ that aren't in a draft-specific directory are compatible with draft2020-12, the latest published version.

This doesn't make much sense, as when running older dialect suites there is no way of knowing which files are the ones that are compatible with that version rather than future ones such that you can process files without potentially processing invalid schemas under the version under test.

I'm not saying this was clear before this PR, but I am saying is this haphazard moving of this over the last few years to directories without holistic care for all of the ecosystem rather than just "it works for my implementation" is really frustrating. Here's Bowtie's ridiculous logic which I bet I'll have to again update after this PR as well, but first I'm focused on my implementation and getting it to be able to run the suite again. (PS this PR also semantically breaks the jsonschema_suite remotes CLI command for the same reasons.)

I've said this politely in the past so I'll say it more strongly -- I'm genuinely shocked every time one of us reviews a PR by saying "this works for me" -- that is simply not the point of a PR review, the point is to use your brain, understand the change, think of whether it seems like a good idea, and try to poke holes in it. Clearly we disagree here, which is one of the reasons I personally have little interest in continuing to invest effort maintaining the suite here in the org.

Julian added a commit to python-jsonschema/jsonschema that referenced this pull request Jan 31, 2025
This new-but-similarly-messy logic comes modified from Bowtie
(which itself may need to be modified there, we'll see).

`jsonschema_suite remotes` is somewhat semantically broken (as it
doesn't really help with not emitting schemas which aren't valid under
the version under test), so it's time to give up and use the other
filesystem mechanism of walking the remotes directory.

Refs: json-schema-org/JSON-Schema-Test-Suite#753
Julian added a commit to python-jsonschema/jsonschema that referenced this pull request Jan 31, 2025
This new-but-similarly-messy logic comes modified from Bowtie
(which itself may need to be modified there, we'll see).

`jsonschema_suite remotes` is somewhat semantically broken (as it
doesn't really help with not emitting schemas which aren't valid under
the version under test), so it's time to give up and use the other
filesystem mechanism of walking the remotes directory.

Refs: json-schema-org/JSON-Schema-Test-Suite#753
Julian added a commit to python-jsonschema/jsonschema that referenced this pull request Jan 31, 2025
This new-but-similarly-messy logic comes modified from Bowtie
(which itself may need to be modified there, we'll see).

`jsonschema_suite remotes` is somewhat semantically broken (as it
doesn't really help with not emitting schemas which aren't valid under
the version under test), so it's time to give up and use the other
filesystem mechanism of walking the remotes directory.

Refs: json-schema-org/JSON-Schema-Test-Suite#753
@karenetheridge
Copy link
Member Author

The motivation is to prevent schemas from being loaded as specification versions they are incompatible with. This way at least an implementation can load a draft7 schema as draft7, rather than assuming it's some other draft and failing to process some keywords. Or, if a draft is older than some implementation's understanding, it can skip the file entirely.

There are indeed some assumptions being made all over the test suite with regards to versions. For example, there are some tests that assume that implementations that understand draft2020-12 also understand draft2019-09. Even if we separated every single document (in both tests/ and remotes/) into a draft-specific directory, these cross-version tests will be outliers and we need to make special consideration for how to communicate to an implementation what is in these tests and whether it is appropriate for it to try to test with them.

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.

4 participants