-
Notifications
You must be signed in to change notification settings - Fork 208
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
betterproto: support Struct
and Value
#551
betterproto: support Struct
and Value
#551
Conversation
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
NB: I haven't made any documentation changes, since I believe this behavior was intended (in terms of Protobuf's specified behavior) but just buggy before. However, if there's somewhere I should note this (like the CHANGELOG?) let me know and I'm more than happy to add it 🙂 Similarly, in terms of semver, I believe this is considered a nonbreaking change (since it's a bugfix). |
Thanks for this! Honestly I think just putting this behaviour on the type itself would be the best solution. The generated file has been pretty stable for a long time so I don't really have a problem just tacking this on at this point. |
No problem! Making sure I understand: by "on the type itself," you mean specialize If so, makes sense -- I can do that in a moment. |
Yep exactly |
...rather than special-casing in the Message ABC. Signed-off-by: William Woodruff <[email protected]>
Alright, moved the specialization to |
Signed-off-by: William Woodruff <[email protected]>
Gentle ping; please let me know if there's anything else I can do here! |
Co-authored-by: James Hilton-Balfe <[email protected]>
Test failures seem unrelated |
Yeah, I think so -- looks like the failure is in |
Triaging a bit more: This suggests that something about type hints is hinky, since |
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
I'd personally stick it in quotes manually rather than using the typing version because it'll be easier to remove after #540 lands |
Signed-off-by: William Woodruff <[email protected]>
(Also, if I can make an additional request: a new beta release after this lands would be greatly appreciated!) |
Can't do a beta release till grpc/grpc#35329 is merged and #540 |
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
@woodruffw @Gobot1234 Is there anything special that needs to be done for serialization/deserialization of the Structs? From what I'm seeing, instantiating a Struct as exemplified in the tests and then attempting to send it over the wire fails because the Struct's dump method is eventually called, which falls back to the Message.dump method, which treats the struct as a dict, which expects all keys to be of type 'string' and all values to be of type 'message' - if the value is an intrinsic type like str, the catch-all bytes(value) call fails in _preprocess_single because of TypeError('string argument without an encoding') |
Oh it seems like it might need special casing for dump yeah |
Yep, thanks for catching that @atomicmac, and sorry for the gap there. I can look into a fix tomorrow. |
I've started work on this in #559, but I'm a little stumped on how to actually specialize |
Thanks for looking @Gobot1234 & @woodruffw ! Yes @woodruffw I was looking at implementing the specialized dump() and came to the same crossroads. It would be nice to not have to wrap every value in the struct, but then autoconverting to protobuf types becomes involved if you want to keep transmitted sizes as small as possible. |
Glad to hear it's not just me 😅 -- I've been thinking about this a bit, and I still don't have a great idea here. |
Summary
Fixes #332. See also #549.
Opening as a draft until I add tests.Checklist