Protobuf API V2: Add map, repeated, type conversions #88
Conversation
seena-stripe
requested review from
kathleen-stripe and
mmoriarity-stripe
and removed request for
mmoriarity-stripe
seena-stripe
force-pushed
the
seena/protobuf-api-v2-composites
branch
from
de14915 to
fbadfe7
Compare
kathleen-stripe
left a comment
kathleen-stripe
left a comment
There was a problem hiding this comment.
A bunch of nits, but nothing major.
Feel free to send back to me for review if you disagree with the nits. Thanks!
| func (r *protoRepeated) Attr(name string) (starlark.Value, error) { | ||
| wrapper, ok := allowedListMethods[name] | ||
| if !ok { | ||
| return nil, nil |
There was a problem hiding this comment.
OOC, is there a reason we don't want to return an error here? The intention/context here might be comment-worthy.
There was a problem hiding this comment.
I had to look this up https://pkg.go.dev/go.starlark.net/starlark?utm_source=godoc#HasAttrs
For implementation convenience, a result of (nil, nil) from Attr is interpreted as a "no such field or method" error. Implementations are free to return a more precise error.
| if side == starlark.Right { | ||
| if _, ok := y.(*starlark.List); ok { | ||
| return starlark.Binary(op, y, r.list) | ||
| } |
There was a problem hiding this comment.
OOC, why is there no case for when y is a protoRepeated here?
There was a problem hiding this comment.
Good question, I gave it a shot and I believe if you have x + y (both lists) it is calling binary on x, left falling to the other case, so we only have to support [] + y for starlark.List on the left side
| ) | ||
|
|
||
| var allowedDictMethods = map[string]func(*protoMap) starlark.Value{ | ||
| "clear": nil, |
There was a problem hiding this comment.
OOC, are there plans to support these functions in the future or is there a reason we're leaving them unimplemented?
There was a problem hiding this comment.
I copied the prior functionality to keep the change small but was also surprised to find "clear": nil means fallback to the underlying starlark.List.Clear
|
|
||
| // FieldDescriptor has the same typeName for []string and string | ||
| // and typeError needs to distinguish setting a []string = int versus | ||
| // appending a value in []string |
There was a problem hiding this comment.
Thanks for this helpful comment! OOC, why can't typeName handle this differentiation? i.e. return []string for lists and string for string.
There was a problem hiding this comment.
I couldn't figure out a cleaner way to do this, but I'm bothered by it. The problem I was running into is that you have the cases:
msg.list_int_field = "a" # TypeError: value "a" (type "string") can't be assigned to "[]int"
msg.list_int_field = ["a"] # TypeError: value "a" (type "string") can't be assigned to "int"But from just the field descriptor typeName doesn't know if we're trying to say "you were trying to set a string on a int (value inside a list)" versus "you were trying to set a string on a field that is a list"
Summary
This PR adds a map and list implementation on top of the go protobuf v2 api, and adds type conversions (
protoreflect.Valueto/fromstarlark.Value). This is largely boilerplate, and is unused so it should not have any functional impact on Skycfg today.protomodule_mapimplementation to wrap an underlyingstarlark.Dictwith typechecking on writes. This lines up withprotoMapfrom the existing implementation https://github.com/stripe/skycfg/blob/trunk/internal/go/skycfg/proto_message.go#L869protomodule_listimplementation to wrap an underlyingstarlark.Listwith typechecking on writes. This lines up withprotoRepeatedfrom the existing implementation https://github.com/stripe/skycfg/blob/trunk/internal/go/skycfg/proto_message.go#L679type_conversions: this implementsvalueFromStarlarkandvalueToStarlarklike the existing implementation (throughout https://github.com/stripe/skycfg/blob/trunk/internal/go/skycfg/proto_message.go)Motivation
I am continuing the migration to the go protubuf v2 api, which is partially started on trunk
Tests
I have added tests for the
mapandlisttypes.type_conversionswill be tested implicitly once the message type is added (it's awkward to access right now). These tests coverTestListMutationandTestMapMutationfrom the existing implementation, but line up with the newprotomodule_testfunctions formessageType,enumandpackageinternal/