-
Notifications
You must be signed in to change notification settings - Fork 16
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
Public encoding.DecodeTypeMeta #52
Public encoding.DecodeTypeMeta #52
Conversation
@@ -165,18 +164,6 @@ func tryFindJson(in []byte) (*json.RawMessage, bool) { | |||
return nil, false | |||
} | |||
|
|||
// DecodeSummary decodes the TypeMeta, ContentEncoding and ContentType fields from the 'Unknown' | |||
// protobuf envelope of the given storage data. | |||
func DecodeSummary(inMediaType string, in []byte, out io.Writer) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand why removing this function better? While DecodeSummary
may not do much, it make sure there is in consistency in printing out summary for batch and non-batch mode. With this change, whenever there is some new meta field we want to display, the change has to be in 2 places, and that would be more error prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function should only be used by cmd without exception, or just move it to cmd 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't see much benefits of this refactoring. DecodeSummary
is closely related to decode function itself, and can be used elsewhere in theory.
@jmhbnz WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's minor pr, though for me it's not particularly clear which high level problem this pr is trying to work towards which is why I have not already added review or approval for it.
I suggest we try focus community effort on progressing towards an agreed direction rather than refactor for the sake of it. Our challenge is obviously that auger has only recently come into sig-etcd and we have not created issues yet to establish direction to go in.
Perhaps we should step back a little from this code diff and try to get agreement on what problems we are wanting to solve so it's clear for future pull requests if they align with the direction we are trying to go in?
If project members or contributors can propose issues to start discussion it would be very helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. I've already made issue #3 describing what I wanted to achieve. which is directly edit the data from etcd, and using it just like kubectl
.
Since the code changes are already a bit much, I'm trying to split it up as much for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this PR is to expose encoding.DecodeTypeMeta
, make it can be used in other packages.
then see that encoding.DecodeSummary
only applies to cmd, so it moves over to cmd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of getting this merged I suggest we limit the pr change to just making DecodeTypeMeta
public. I am not in favor of the change to DecodeSummary
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. change it as you say.
I am not in favor of the change to DecodeSummary.
And can you tell me why? As I understand DecodeSummary
will never be used by others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally because it is so early in lifecycle of auger within sig-etcd. We don't yet have regular releases occurring, or announcements of deprecations, or clear understandings of all use cases external to the project. For these reasons I am wary of removing a previously public function just for a very optional refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, I finally understand the difference in how we think about this, you see it as a legacy project that needs to ensure compatibility, and I see it as a new project, the history is actually in github.com/jpbetz/auger.
9268c1d
to
3ea5223
Compare
Signed-off-by: Shiming Zhang <[email protected]>
3ea5223
to
f3ea259
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Thanks @wzshiming
Relates to #3