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

Add lossy64BitNumberRepresentations option to JsonFormat.Printer #2214

Closed
wants to merge 1 commit into from

Conversation

mickeyreiss
Copy link

Here's my WIP for addressing #1823. @xfxyjwf let me know if you'd be interested in accepting this or something similar, and I'd be happy to clean it up/fix tests, whatever it takes.

I don't love this change on principle, but it will help my organization out with backwards compatibility.

Thanks!

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Sep 30, 2016

In principle we would like minimize the number of custom options on the JsonFormat class because the more options we have, the harder it is to make all implementations conform to the same proto3 JSON spec. In this particular case, it's not just one more option, but also an option that breaks interoperability between different proto3 JSON implementations. For this reason we probably won't accept any such changes.

I don't have a great answer to your migration problem though. You probably need to find some other workaround without direct support from protobuf JsonFormat class. Sorry for that...

@xfxyjwf xfxyjwf self-assigned this Sep 30, 2016
@mickeyreiss
Copy link
Author

mickeyreiss commented Sep 30, 2016

Thanks for taking a look. You make a great point about conformant JSON implementations.

Would you be open to opening up the JsonFormat.Printer class to extension via subclassing?

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Mar 20, 2018

This is a very late comment but we do not want to allow subclassing either. The idea is to force everyone to use a standard format that's easy to communicate across different implementations/languages. Giving users ability to customize the format will defeat this purpose (unless the customization is part of the spec and implemented everywhere).

@jonny-improbable
Copy link

@xfxyjwf Thanks for commenting on this issue; it's regrettable that there is still no solution for preserving int64 values when using JavaScript (also see #1832) - this is a big problem for a lot of teams as large numeric values are not preserved from client <> server.

Changing int64 to be represented by a string instead of a number would be a breaking change, but is the only way teams can have any confidence when using javascript clients.

Please could one of the core maintainers provide a path forward, or at least some input, on this issue?

Thanks

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Mar 26, 2018

@jonny-improbable Could you help create an issue for it? I can help assign the issue to other team members.

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.

5 participants