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

Deprecate JsonElement constructor #1761

Merged

Conversation

Marcono1234
Copy link
Collaborator

Follow-up for #1689.

Creating custom JsonElement subclasses is discouraged and will lead to issues when using JsonTreeReader.

Creating custom JsonElement subclasses is discouraged.
@google-cla google-cla bot added the cla: yes label Aug 22, 2020
@Marcono1234
Copy link
Collaborator Author

@eamonnmcmanus, could you please have a look?

@eamonnmcmanus
Copy link
Member

Thanks! I agree with the principle. I had a couple of small comments.
Another approach that would avoid having so many @SuppressWarnings("deprecation") annotations would be to have a second, package-private JsonElement constructor with a dummy parameter, and use that in the subclasses. But I think @SuppressWarnings is probably better.

gson/src/main/java/com/google/gson/JsonArray.java Outdated Show resolved Hide resolved
reader.peek();
fail();
} catch (MalformedJsonException expected) {
assertEquals("Custom JsonElement subclass com.google.gson.internal.bind.JsonTreeReaderTest$1CustomSubclass is not supported",
Copy link
Member

Choose a reason for hiding this comment

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

I think I would write:

assertEquals("Custom JsonElement subclass " + CustomSubclass.class.getName() + " is not supported", ...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, that hardcoded generated name JsonTreeReaderTest$1CustomSubclass would likely cause problems in the future.

@eamonnmcmanus
Copy link
Member

Experimentally, I tried just changing the JsonElement constructor to package-private and testing that against all of Google's internal code. There were no failures. So, even though it is technically an incompatible API change, I think maybe we could allow ourselves to make it. Clearly the existing default public constructor was an oversight.

@Marcono1234
Copy link
Collaborator Author

So, even though it is technically an incompatible API change, I think maybe we could allow ourselves to make it. Clearly the existing default public constructor was an oversight.

To be safe, I think keeping the constructor for now might be better because at least the Auto.js project has a custom subclass. Also, unlike some other issues, keeping the deprecated JsonElement constructor would probably not be such a big problem; and we can still remove it in the future.

@eamonnmcmanus
Copy link
Member

OK, given that we know of one subclass I agree that the change here is the right way to go.

@eamonnmcmanus eamonnmcmanus merged commit 0b6a7bf into google:master Aug 4, 2022
@Marcono1234 Marcono1234 deleted the marcono1234/json-element-subclass branch August 5, 2022 21:19
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.

2 participants