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

Create a guide for proto.Message String() methods #12908

Closed
Tracked by #13085
robert-zaremba opened this issue Aug 11, 2022 · 5 comments · Fixed by #13364
Closed
Tracked by #13085

Create a guide for proto.Message String() methods #12908

robert-zaremba opened this issue Aug 11, 2022 · 5 comments · Fixed by #13364
Assignees
Labels
T: Dev UX UX for SDK developers (i.e. how to call our code) T:Docs Changes and features related to documentation.

Comments

@robert-zaremba
Copy link
Collaborator

Summary

  • in some legacy SDK Message implementations we are using yaml Marshaller.
  • Moreover ecosystem code uses that method for pretty-printing
  • This is buggy, direct yaml marshalling is not supported any more, can produce wrong output or not display some fields at all (eg sdk.Uint). One workaround is to go through JSON (msg -> json -> map -> yaml)

Proposal

Create a guide or an antipattern guide explaining proper way to format proto Messages.

Ref: #12880 (comment)

@robert-zaremba robert-zaremba added T:Docs Changes and features related to documentation. T: Dev UX UX for SDK developers (i.e. how to call our code) labels Aug 11, 2022
@alexanderbez
Copy link
Contributor

What do you mean by "format Proto messages"?

@robert-zaremba robert-zaremba changed the title Create a guide for proto Message String() methods Create a guide for proto.Message String() methods Aug 11, 2022
@robert-zaremba
Copy link
Collaborator Author

What do you mean by "format Proto messages"?

Creating human friendly representation of a proto.Message, usually it's done in the String() method.

@aaronc
Copy link
Member

aaronc commented Aug 11, 2022

My opinion is that developers shouldn't be implementing proto message String methods by hand. The official proto compiler does it automatically. It's only because of some gogo options this is even allowed

@alexanderbez
Copy link
Contributor

I'm struggling to understand the proposal. Isn't this just making sure we don't include option (gogoproto.goproto_stringer) = false;?

@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Aug 11, 2022

@alexanderbez the proposal is to create a guide / recommendation, in an documentation. Because as we can see, it's clear for developers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Dev UX UX for SDK developers (i.e. how to call our code) T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants