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

Update unevaluatedProperties.json #743

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MeastroZI
Copy link
Contributor

#723

I think removing the bar matches the test with the description, as foo itself is evaluated.

json-schema-org#723

 think removing the bar matches the test with the description, as foo itself is evaluated.
@MeastroZI MeastroZI requested a review from a team as a code owner April 26, 2024 12:15
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.

It would be better to just add a test rather than change one. This test has purpose.

@MeastroZI
Copy link
Contributor Author

MeastroZI commented Apr 26, 2024

But then description need to be change for this

 {
                "description": "when if is true and has no unevaluated properties",
                "data": {
                    "foo": "then",
                    "bar": "bar"
                },
                "valid": false
            }

as bar is unevaluated , if we change the description it will become duplicate of other test

@MeastroZI
Copy link
Contributor Author

If i am not wong currently this test is duplicate of this one

 {
                "description": "when if is true and has unevaluated properties",
                "data": {
                    "foo": "then",
                    "bar": "bar",
                    "baz": "baz"
                },
                "valid": false
 },

@jdesrosiers
Copy link
Member

I think @MeastroZI was right the first time. The original test should be modified because it's just a duplicate of another test. Changing the test makes it cover a unique case and what it was testing is already covered elsewhere.

@gregsdennis
Copy link
Member

No worries. I was on mobile and hadn't checked other tests. Sorry for the confusion.

@gregsdennis gregsdennis requested review from a team and removed request for gregsdennis April 30, 2024 21:07
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.

None yet

3 participants