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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
wzshiming
force-pushed
the
feat/decode-type-meta
branch
2 times, most recently
from
9268c1d
to
3ea5223
Compare
Signed-off-by: Shiming Zhang <wzshiming@foxmail.com>
wzshiming
force-pushed
the
feat/decode-type-meta
branch
from
3ea5223
to
f3ea259
Compare
Relates to #3