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

add a test case for an enum implementing JSONString #870

Closed
wants to merge 2 commits into from

Conversation

Simulant87
Copy link
Contributor

@Simulant87 Simulant87 commented Feb 25, 2024

This test case would fail when the instanceof check for JSONString in the writeValue method of JSONObject would be moved to the bottom below the check for enum.

@stleary
Copy link
Owner

stleary commented Mar 9, 2024

What is the purpose of this change?

@Simulant87
Copy link
Contributor Author

When the ordering of the instanceof checks in the writeValue method is changed, so that the instanceof enum check if done before the instanceof JsonString, it changed unexpectedly the behaviour of the method, without any failing test. This new text would fail on the described change, ensuring that this expected behaviour does not break.

@stleary
Copy link
Owner

stleary commented Mar 9, 2024

Is this change related to #863? If so, it should be added to the existing PR.

@Simulant87
Copy link
Contributor Author

Simulant87 commented Mar 9, 2024

I noticed this gap in the test coverage, while working on #863. So I can add it to that branch.

I just thought you could merge this test already independently of it, as we still an active discussion on the other branch, and the added test case is also valid without the other changes on #863. But if you prefer to have it on the other branch for context, I will add it there.

@Simulant87
Copy link
Contributor Author

Simulant87 commented Mar 10, 2024

I merged it to #867 , which is the branch for ticket #863, so if that PR will be accepted, this PR could be closed.

@stleary stleary removed the In review label Mar 17, 2024
@stleary
Copy link
Owner

stleary commented Mar 17, 2024

Closing since the code was moved to #867

@stleary stleary closed this Mar 17, 2024
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.

2 participants