Allow reading json_name field option for proto serialization
#2701
Conversation
…` method In the other commits in this PR, I plan to introduce branching logic inside of the customization of the serialized name for fields. This change is a pure refactor that serves to isolate the business logic into a separate commit so as to make it easier to understand.
There was a problem hiding this comment.
Overall this looks great. Just some relatively minor comments.
I'm not sure if @Marcono1234 also wants to weigh in on this review.
proto/src/main/java/com/google/gson/protobuf/ProtoTypeAdapter.java
Outdated
Show resolved
Hide resolved
| * Sets or unsets a flag that, when set, causes the adapter to use the `json_name` field option | ||
| * from a proto field for serialization. This `json_name` option is an annotation applied to | ||
| * proto fields that cannot be read from the descriptor's options | ||
| * (`FieldDescriptor::getOptions`) as it is only available via `FieldDescriptor::getJsonName`. |
There was a problem hiding this comment.
Here I think you could write {@link FieldDescriptor#getOptions} and {@link FieldDescriptor#getJsonName} since we do import FieldDescriptor.
There was a problem hiding this comment.
Is this sentence about getOptions and getJsonName actually relevant for users of this adapter, or is this an implementation detail and we should omit it?
There was a problem hiding this comment.
i meant it as a way to explain why users of the adapter expect to see this method here instead of just using the addSerializedNameExtension which might be their initial intuition. i'm cool with removing it if you think it doesn't help.
proto/src/main/java/com/google/gson/protobuf/ProtoTypeAdapter.java
Outdated
Show resolved
Hide resolved
...src/test/java/com/google/gson/protobuf/functional/ProtosWithAnnotationsAndJsonNamesTest.java
Outdated
Show resolved
Hide resolved
proto/src/main/java/com/google/gson/protobuf/ProtoTypeAdapter.java
Outdated
Show resolved
Hide resolved
...src/test/java/com/google/gson/protobuf/functional/ProtosWithAnnotationsAndJsonNamesTest.java
Outdated
Show resolved
Hide resolved
...src/test/java/com/google/gson/protobuf/functional/ProtosWithAnnotationsAndJsonNamesTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
I'm not sure if @Marcono1234 also wants to weigh in on this review.
I have added just one small review comment. Though I am not that familiar with the Protobuf code here and Protobuf in general, but @jabagawee what you described in the issue sounds reasonable. Thanks for the extensive explanation and tests!
...src/test/java/com/google/gson/protobuf/functional/ProtosWithAnnotationsAndJsonNamesTest.java
Outdated
Show resolved
Hide resolved
Purpose
Closes #2700
Description
Explained in #2700
Checklist
This is automatically checked by
mvn verify, but can also be checked on its own usingmvn 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.null@since $next-version$(
$next-version$is a special placeholder which is automatically replaced during release)TestCase)mvn clean verify javadoc:jarpasses without errors