-
-
Notifications
You must be signed in to change notification settings - Fork 231
Fix unevaluatedProperties with adjacent bool additionalProperties test case
#793
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?
Fix unevaluatedProperties with adjacent bool additionalProperties test case
#793
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.
Thanks for catching that! Can you make this change in the 2019-09 and v1 tests as well.
|
I think this is the only draft with this test? $ command grep -nire 'unevaluatedProperties with adjacent bool additionalProperties'
tests/draft2020-12/unevaluatedProperties.json:130: "description": "unevaluatedProperties with adjacent bool additionalProperties",It looks like 2019 has |
|
Oh, that's weird. There's no reason there should be different tests in those three versions. It looks like someone broke that test in 2020-12, but didn't make the change in the other versions. We need to make all three the same. I think the best approach is to use the 2019-09 version of the schema/tests and the 2020-12 version of the description. Then we need to copy the "unevaluatedProperties with adjacent non-bool additionalProperties" test to 2019-09 and v1. |
This fixes the disparaty between then 2019, 2020, and v1 drafts, and adds the property constraint back into the 2020 schema, so that the first test case matches its description.
6b8885f to
e0e431f
Compare
|
@jdesrosiers Alright, I've updated all three drafts to match, with the Note that the |
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.
Oh, I wasn't expecting you to change the non-bool test, just copy it over to the other versions. But, this way is better. The test for an invalid additional property in the original version was testing additionalProperties, not unevaluatedProperties.
Thanks for helping clean this up. Since we're changing an existing test, I'm going to leave this open for a little while to give more people a chance to review.
Unless I'm missing something, it looks like there's an additional property in the
with no additional propertiestest case of theunevaluatedProperties with adjacent bool additionalPropertiessuite.The case with additional properties is the next one.