Skip to content

Conversation

Vinit-Pandit
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.
@Vinit-Pandit Vinit-Pandit 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.

@Vinit-Pandit
Copy link
Contributor Author

Vinit-Pandit 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

@Vinit-Pandit
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 dismissed their stale review April 30, 2024 21:06

resolved.

@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.

3 participants