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

Fixing issue #1592 #1620

Closed
wants to merge 0 commits into from
Closed

Fixing issue #1592 #1620

wants to merge 0 commits into from

Conversation

fmmmlee
Copy link
Contributor

@fmmmlee fmmmlee commented Nov 22, 2019

As cqjason describes in issue #1592, the behavior of JsonTreeWriter.name() is inconsistent with the method it overrides in its parent JsonWriter. When calling JsonWriter.name(null), a NullPointerException is thrown, while calling JsonTreeWriter.name(null) does not throw an exception, leading to a potentially misleading IllegalStateException when calling JsonTreeWriter.value() later in execution.

To resolve this, I just copied the null check from lines 385-387 of JsonWriter.

Edit: I had made a few style changes to the design document on this fork, but reverted them in order to keep the changes issue-related.

@Marcono1234
Copy link
Collaborator

Would it be possible to force push only 49280df or squash the commits? I assume the maintainers here would like to not include commits which are then reverted in a later commit.

@fmmmlee
Copy link
Contributor Author

fmmmlee commented Nov 26, 2019

I believe maintainers have the option to squash when merging a pull request, so it only shows up as a single commit. I don't think they can be squashed before merging, though; only when performing the merge.

Edit: Closed issue and opened #1623 that only has the relevant commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants