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

Rename codec.Marshaler #8413

Closed
4 tasks
robert-zaremba opened this issue Jan 22, 2021 · 2 comments · Fixed by #9226
Closed
4 tasks

Rename codec.Marshaler #8413

robert-zaremba opened this issue Jan 22, 2021 · 2 comments · Fixed by #9226
Assignees
Labels
C:Encoding T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Milestone

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Jan 22, 2021

Summary

codec.Marshaler and codec.ProtoMarshaler have similar name but totally different responsibility.

Problem Definition

codec.ProtoMarshaler, similar to json.Marshaler, is for self marshaling.
codec.Marshaler, on the other hand, is for marshaling other objects. Reading the name I would assume that both are of the same kind, but they have completely different responsibility.

Proposal

Two options:

  • rename codec.ProtoMarshaler to codec.Coder
  • or remove the interface since we can use proto.Marshal -> ref: Use proto.Marshal for store objects Marshaling #8412
    • Note - we will need to move some helper methods out of the implementation to the codec package: methods related to interface (Any) marshaling.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). C:Encoding labels Jan 22, 2021
@aaronc
Copy link
Member

aaronc commented Apr 28, 2021

How about we rename codec.Marshaler -> codec.Codec since the two implementations are ProtoCodec and AminoCodec. And similarly BinaryMarshaler and JSONMarshaler become BinaryCodec and JSONCodec.

@robert-zaremba
Copy link
Collaborator Author

So, object which serializes other objects will be called Codec*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Encoding T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants