Skip to content

Conversation

@MukjepScarlet
Copy link
Contributor

Purpose

Currently some of APIs use Appendable as parameter and some other use Writer as parameter. This PR migrates all Writer parameters into Appendable. No effect for normal usages.

Description

closes #2924
Also adjusts some documentations.

Checklist

  • New code follows the Google Java Style Guide
    This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
    Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
  • If necessary, new public API validates arguments, for example rejects null
  • [] New public API has Javadoc (I don't know if these are "new" APIs)
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

Copy link
Contributor

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No effect for normal usages.

This only applies to source compatibility. However, changing the parameter type breaks binary compatibility (the "Check API compatibility" CI workflow highlights this): Code which has been compiled against an older Gson version will still resolve against the methods and constructors with Writer parameter, and will therefore break when a newer Gson version with this change is used.

So it would be necessary to keep the existing methods and constructors with Writer parameter, and instead add additional ones taking an Appendable.

Personally I am not that convinced by this change. #2924 mentions as use case new JsonWriter(new StringBuilder)) but users could use a StringWriter instead of a StringBuilder.
And as mentioned above, having to keep the existing overloads with Writer would make the API then a bit verbose.

Maybe we should wait for more users to raise this issue, and see why the current API with Writer parameters is not suitable (and why implementing a custom Writer is not an option).

This is just my personal opinion on this though; I am not a direct member of this project.
Either way, thanks for this PR!

@MukjepScarlet
Copy link
Contributor Author

This only applies to source compatibility.

I understand your meaning. My no effect means users can upgrade the lib without changing their code. I will add the old ones back and add comment for them.

@MukjepScarlet
Copy link
Contributor Author

@Marcono1234 This is because the JsonWriter don't depend on Writer API. About the reason, Writer has builtin synchronized, but I think 99% of JSON use cases don't need it. Streams is more likely an internal API.
Also, StringReader has synchronized too. I might replace it with a custom one in another PR.

@Marcono1234
Copy link
Contributor

I don't think the existing constructors and methods with Writer parameter should be deprecated. There is nothing broken or error-prone with them, so the deprecations just cause noise for users. Additionally using the new Appendable overload would require the users to do an (Appendable) ... cast just to avoid the deprecation warning, which is rather verbose.


Also, StringReader has synchronized too. I might replace it with a custom one in another PR.

For this one, definitely create an issue first please to discuss this. Especially because there is no equivalent base interface (as it is the case for Writer and Appendable) and because JDK 24 added Reader.of(CharSequence). So I don't think this is necessary.

@google google deleted a comment from marykluther-glitch Oct 28, 2025
@MukjepScarlet
Copy link
Contributor Author

I think so. The deprecation will make it annoying, as a Writer param will auto use the old constructor.

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.

Use Appendable instead of Writer as the parameter of JsonWriter

2 participants