Adding encoding support for Protocol Buffers #127
Conversation
|
@philippgille please review |
glimchb
force-pushed
the
master
branch
3 times, most recently
from
5428a3f
to
b9f0d49
Compare
|
Thank you! I'll have a look on Tuesday. |
glimchb
force-pushed
the
master
branch
2 times, most recently
from
0df89e7
to
f8148c8
Compare
There was a problem hiding this comment.
Thank you very much for your contribution!
My two main concerns are the dependency that affects every user (even the ones using only another codec), and the difference to other codecs being that it doesn't support every Go type but only proto messages.
But I'm open to discuss it in the related comments!
Do you have a usage of this codec in mind, maybe in a project you're working on? Do you already have this codec implemented in another place and would like to centralize it in the gokv repo?
There was a problem hiding this comment.
The JSON and Gob codecs don't have tests in this package, and instead some store specific tests tests use both codecs (as mentioned and linked in the other comment), but maybe we should change that. Stores shouldn't have to test the different codecs, and the codecs should have some tests on their own. WDYT?
There was a problem hiding this comment.
Yes, agree 100%. codecs need to have their own test like #103 already does, I can add tests for this proto codec as well
There was a problem hiding this comment.
Yes that would be great! Thanks!
|
Another question:
The issue mentions the use of the |
Unless I'm missing something, it make sense to encode to wire format and not to json, that's why I used |
There was a problem hiding this comment.
Yes that would be great! Thanks!
README.md
Outdated
| @@ -128,6 +129,7 @@ Differences between the formats: | |||
| - Depending on the use case, the custom (un-)marshal methods of one of the formats might be easier to implement | |||
| - JSON: [`MarshalJSON() ([]byte, error)`](https://pkg.go.dev/encoding/json#Marshaler) and [`UnmarshalJSON([]byte) error`](https://pkg.go.dev/encoding/json#Unmarshaler) | |||
| - gob: [`GobEncode() ([]byte, error)`](https://pkg.go.dev/encoding/gob#GobEncoder) and [`GobDecode([]byte) error`](https://pkg.go.dev/encoding/gob#GobDecoder) | |||
| - proto: [`Marshal(proto.Message) ([]byte, error)`](https://pkg.go.dev/google.golang.org/protobuf/encoding/protojson#Marshal) and [`Unmarshal([]byte, proto.Message) error`](https://pkg.go.dev/google.golang.org/protobuf/encoding/protojson#Unmarshal) | |||
There was a problem hiding this comment.
With the PR addressing proto (un)marshalling and not protojson, are these the right links?
Yes definitely! Then let's either change that ticket to refer to proto and not protojson, or in the PR description here remove the "Fixes |
According to proto documentation, protojson produces a different output than the standard "encoding/json" package, which does not operate correctly on protocol buffer messages. Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
There was a problem hiding this comment.
Thanks for the changes!
The unit tests are still missing, but I think we can add them in a follow-up PR.
Same as moving the codec to a subpackage with its own go.mod file so that users of the encoding package don't end up with any proto transitive dependencies when they don't use anything proto related.
Thanks again 💪
|
thanks @philippgille ! much appreciated |
According to proto documentation,
protojson produces a different output than the standard "encoding/json" package,
which does not operate correctly on protocol buffer messages.
Fixes #128