-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add a proto.to_any method #83
Add a proto.to_any method #83
Conversation
The build is failing but I'm confused as to why given the dependencies don't appear to have changed. |
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.
Looks generally good -- two minor comments.
internal/go/skycfg/proto_api.go
Outdated
"go.starlark.net/starlark" | ||
yaml "gopkg.in/yaml.v2" | ||
"gopkg.in/yaml.v2" |
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.
Can you keep this import alias as-is? It's nice to have explicitly named imports when upstream uses a non-default package name.
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.
I think this is a holdover/cargo-cult from the first iteration of this change. Let me change the alias back.
internal/go/skycfg/proto_api.go
Outdated
return nil, err | ||
} | ||
|
||
// Disambiguate between golang encoded proto messages. |
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.
I'm not sure why this branch is needed. Why not any, err := ptypes.MarshalAny(msg.msg)
? Was there a case you encountered where a protobuf message has no type name?
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.
I think this is a holdover/cargo-cult from the first iteration of this change. I'll remove it.
It looks like something in We should probably drop |
Likely remnant of the pr this is based on - it used this check to
differentiate between gogo and standard registries (which I’m not sure if
they are actually equivalent or not)
…On Tue, Sep 29, 2020 at 9:05 PM John Millikin ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Looks generally good -- two minor comments.
------------------------------
In internal/go/skycfg/proto_api.go
<#83 (comment)>:
> "go.starlark.net/starlark"
- yaml "gopkg.in/yaml.v2"
+ "gopkg.in/yaml.v2"
Can you keep this import alias as-is? It's nice to have explicitly named
imports when upstream uses a non-default package name.
------------------------------
In internal/go/skycfg/proto_api.go
<#83 (comment)>:
> @@ -236,6 +238,30 @@ func fnProtoToJson(t *starlark.Thread, fn *starlark.Builtin, args starlark.Tuple
return starlark.String(jsonData), nil
}
+// Implementation of the `proto.to_any()` built-in function. Returns a
+// skyProtoMessage with an `Any` proto.Message in it.
+func fnProtoToAny(t *starlark.Thread, fn *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+ var msg *skyProtoMessage
+ err := wantSingleProtoMessage(fn.Name(), args, kwargs, &msg)
+ if err != nil {
+ return nil, err
+ }
+
+ // Disambiguate between golang encoded proto messages.
I'm not sure why this branch is needed. Why not any, err :=
ptypes.MarshalAny(msg.msg)? Was there a case you encountered where a
protobuf message has no type name?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#83 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAF3KAEPKGAJITNWBS6BUBLSIKVCHANCNFSM4R5JSJPA>
.
|
Updated the conditional and import, and dropped ptal @jmillikin-stripe |
7390795
to
4d54442
Compare
This PR uses the code from #46 and responds to the review feedback on this PR.
I've removed the gogo code entirely to be consistent with the rest of the provided functions in
proto_api.go
, which don't support gogo protos.This PR is also quite old so I've also rebased it onto master.