stripe / skycfg

Skycfg is an extension library for the Starlark language that adds support for constructing Protocol Buffer messages.
Apache License 2.0
643 stars 54 forks source link

Add a proto.to_any method #83

Closed idiamond-stripe closed 3 years ago

idiamond-stripe commented 3 years ago

This PR uses the code from https://github.com/stripe/skycfg/pull/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.

idiamond-stripe commented 3 years ago

r? @jmillikin-stripe

The build is failing but I'm confused as to why given the dependencies don't appear to have changed.

jmillikin-stripe commented 3 years ago

The build is failing but I'm confused as to why given the dependencies don't appear to have changed.

It looks like something in Go: master is unhappy with the contents of go.mod. No idea why, but since it's not affecting any stable release I think it's fine to ignore.

We should probably drop Go: master from the CI suite since I don't see an obvious way to make Travis test failures informational.

dilyevsky commented 3 years ago

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 notifications@github.com wrote:

@jmillikin-stripe requested changes on this pull request.

Looks generally good -- two minor comments.

In internal/go/skycfg/proto_api.go https://github.com/stripe/skycfg/pull/83#discussion_r497230787:

"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 https://github.com/stripe/skycfg/pull/83#discussion_r497231023:

@@ -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 https://github.com/stripe/skycfg/pull/83#pullrequestreview-499069833, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAF3KAEPKGAJITNWBS6BUBLSIKVCHANCNFSM4R5JSJPA .

idiamond-stripe commented 3 years ago

Updated the conditional and import, and dropped go master.

ptal @jmillikin-stripe