-
Notifications
You must be signed in to change notification settings - Fork 806
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
Work In Progress: casttype (new type of customtype) #36
Comments
custom type only works on bytes. I think this needs clearer documentation
|
Maybe this could be a feature request. |
I see what you're saying. My expectation, trying to use it, was that if the proto side of things says "uint32" and my type is just a type T uint32, it would marshal like an uint32 uint32 would have ;) It's already really close; all it seems to need is a type conversion around that |=. (And then non-assignable type T's will crap out on a different weird error, but that's a different problem; that can't really be avoided without protoc-gen-gogo trying to parse the Go source, I think.) If I understand what you're saying, you're implying this is undesirable because what is a uint32 in Go might on the wire be represented by just about anything; maybe the Go variable is e.g. just a lookup key to var cache map[uint32]string, and the strings are the actual on-wire content. I see that, but also in this case the protobuf type was uint32, and that seems like a sane default to me. I understand that implementation is more complex. Also, just to avoid misunderstand, there's no such thing as MarshalUint32 currently, right? I'm wary of http://godoc.org/github.com/gogo/protobuf/proto#Marshaler because as far as I know it ends up on the wire as (protobuf type bytes, or it fails, right?), and my data is just an uvarint in itself. As a workaround, I set gogoproto.unmarshaler=false for that message, that made it work. That particular message wasn't performance critical for me, for now, anyway. I'll loop back to this problem when my code is functionality-wise in better shape, and I can start worrying about performance again ;) Thank you. |
Yes I don't want protoc-gen-gogo to parse Go source. It does seem like a sensible default. I don't know how much effort this would be to implement though. Yes there is no such thing as MarshalUint32 currently and I also don't think that would be a good idea for the future. Conclusion: I think issue 28 should be sorted out first. |
It seems just changing m.X |= (uint32(b) & 0x7F) << shift to m.X |= T((uint32(b) & 0x7F) << shift) would be enough. I'm not sure if you want to keep this separate from |
Oh and MarshalBinary won't really solve this, because the wire format is different when the .proto says |
I agree MarshalBinary does not solve this, it might just make this use case less important. |
We should also think about what happens when the type is another message and the field has a customtype. For example: message A { message Uuid { I have seen this exact use case in the wild. This means that the go client would prefer just to work with the uuid type, but another client prefers the message type. |
See PR #46 for a stab at fixing customtype when used with integer wire-types. It's not quite complete as behavior of integer wire types with a customtype differs if a generated marshaler/unmarshaler is used vs the reflection-based ones. The reflection-based marshal/unmarshal will use the customtypes Somewhat problematically, the behavior I'm proposing would be a backwards incompatible change as customtype does work with integer wire-types if you're using reflection-based marshaling and unmarshaling. |
#46 works only if the custom type is an alias for an integer type (i.e. The use case I have in mind is our |
What do you think about |
@bdarnell you can cast from one struct to another if the fields are identical: http://play.golang.org/p/Kl2iJrKIto |
Types and names must both match, so the more cumbersome |
@bdarnell For portability to other languages I think the wire-type encodings should not be tampered with. Custom types for integer wire-types should probably have
|
I hope to get a chance on Monday to think about this again. |
Unfortunately I did not get a chance. |
I too needed to serialize aliased int types, so I made some modifications just as a proof of concept (PR #50) for generated unmarshal code. However, here is my rationale of different type structures, using int64 as an example:
The idea is that gogoproto.customtype is no more an option that means "use (un)marshal methods", but an option that means "use this go type, using different methods":
I am not sure I made myself clear :) But if I'm not mistaken that seems to match the use cases. |
Ok I propose a new customtype called casttype. |
Sorry for the super long delay. |
Any volunteers? |
Ok so no volunteers, thats fair :) |
Sorry I was in holidays :) I'm afraid I am not up for the task, considering I'm not familiar enough with the project. |
Ok great thats one up vote :) |
I really want to clean up some on these long pending issues. If someone would try to use it and see if it breaks it would be greatly appreciated. Here is an example:
|
The casttype branch now contains all the new features that have recently been added and the bug that has been fixed. |
Perfect ! Thank you for this work. I hope to test it as soon as I have some time in the next weeks. |
cockroachdb seems to be having success with casttype :) |
merged to master :) |
Yes we do :) Briefly tested it, so far so good. I had no time to use it thoroughly for production yet, but will do! Thanks! |
Good to hear :) |
main.go:
repro.proto:
The text was updated successfully, but these errors were encountered: