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.

Sign up for GitHub

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

Jump to bottom

fixes customizer logic in the GsonGrpcJsonMarshallerBuilder #6146

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fixes customizer logic in the GsonGrpcJsonMarshallerBuilder #6146

wants to merge 4 commits into from

Conversation

AnyRoad
Copy link

@AnyRoad AnyRoad commented Mar 9, 2025

Motivation:

  • JsonFormat.Parser and JsonFormat.Printer classes are immutable and call of any method in customizer of the GsonGrpcJsonMarshallerBuilder creates new object.
  • Because of that result of the customer is ignored so we need a way to use new object created in the customizer.

Modifications:

  • Replaces Consumer which does not return anything with the Function<T, T> customizer which returns customized object.

Result:

  • We can actually customize JsonFormat.Parser and JsonFormat.Printer using the customizer object.

@AnyRoad
fixes customizer logic in the GsonGrpcJsonMarshallerBuilder
97720ee 
JsonFormat.Parser and JsonFormat.Printer classes are immutable and
calling any method in customizer creates new object.
Because of that result of the customer is ignored so we need a way
to use new object created in the customizer.
Replaces Consumer<T> with the Function<T, T> customizer which
returns customized object.
@CLAassistant
Copy link

CLAassistant commented Mar 9, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 9, 2025 •
edited
Loading

🔍 Build Scan® (commit: d387e3b)

Job name Status Build Scan®

@minwoox
Copy link
Contributor

minwoox commented Mar 10, 2025

Hi, @AnyRoad! Thanks for the PR!
Would you mind:

  • Signing CLA
  • Adding test cases that verities the differences?
  • Adding a new method instead of changing the signature of the existing method which breaks the compatibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon Awaiting requested review from ikhoon ikhoon is a code owner

@jrhee17 jrhee17 Awaiting requested review from jrhee17 jrhee17 is a code owner

@minwoox minwoox Awaiting requested review from minwoox minwoox is a code owner

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AnyRoad @CLAassistant @minwoox