Skip to content
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

Support "bytes" proto (v2/v3) type. #13

Merged
merged 3 commits into from Nov 27, 2018
Merged

Support "bytes" proto (v2/v3) type. #13

merged 3 commits into from Nov 27, 2018

Conversation

dilyevsky
Copy link
Contributor

I ran into this when trying to set .data on corev1.Secret message (it is of type map<string, bytes>). Since Starlark lacks bytes type like Python I think it's best to just implicitly convert this to string type.

Copy link
Contributor

@jmillikin-stripe jmillikin-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good! I was surprised to see the Starlark strings are equivalent to Python's bytes instead of str: https://github.com/bazelbuild/starlark/blob/master/spec.md#strings

Can you also add the new test field to test_proto_gogo.proto? I don't expect gogo does anything different from go-protobuf here, but it's good to keep them similar because it makes the true differences stand out more.

var err error
// If dst is []byte ([]Uint8) and src is string, attempt to
// convert value below.
if t.Kind() == reflect.Slice && t.Elem().Kind() == reflect.Uint8 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic feels like it belongs in scalarFromStarlark(). Since the Go type is passed in already, you can special-case the conversion from string to []uint8 and return a reflected slice (which should be assignable).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dilyevsky
Copy link
Contributor Author

dilyevsky commented Nov 27, 2018

Can you also add the new test field to test_proto_gogo.proto?

Also done.

PTAL

Copy link
Contributor

@jmillikin-stripe jmillikin-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jmillikin-stripe
Copy link
Contributor

I'm going to force the merge past test errors because it got stuck on a bad base commit.

@jmillikin-stripe jmillikin-stripe merged commit 4bd5168 into stripe:master Nov 27, 2018
@dilyevsky dilyevsky deleted the feature/bytes-support branch November 27, 2018 22:01
@dilyevsky
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants