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

gogo support #66

Merged
merged 16 commits into from
Mar 27, 2018
Merged

gogo support #66

merged 16 commits into from
Mar 27, 2018

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Mar 17, 2018

This is a working prototype for implementing constraints for gogo-generated protobufs.
This is sufficient for handling envoyproxy/data-plane-api:

  1. supports gogoproto.nullable annotation
  2. supports gogoproto.stdduration annotation
  3. supports gogoproto.stdtime annotation, or any combination thereof

Signed-off-by: Kuat Yessenov [email protected]

Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

Partially addresses #52

@kyessenov
Copy link
Contributor Author

@rodaine I want to hear your thoughts how best to structure this. I can fix the build once we agree how to get this started.

Copy link
Member

@rodaine rodaine 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 awesome! Can we also add docs to identify support as well as what features of gogo's annotations will work (or more specifically which ones won't)?

if err != nil { return {{ errCause . "err" "value is not a valid duration" }} }
{{ end }}
{{ end }}
{{ else }}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of these big conditionals, how about we move the shared templates into something like goshared and have separate templates for where the two targets diverge? So everything but duration, time, and file (or perhaps just a preamble) are shared. Does that make sense?

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

@@ -0,0 +1,490 @@
// Code generated by protoc-gen-gogo.
Copy link
Member

Choose a reason for hiding this comment

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

Please exclude the test gen'd code

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

google.protobuf.Duration gogo1 = 17 [(validate.rules).duration.lt = {seconds: 5}, (gogoproto.stdduration) = true];
google.protobuf.Duration gogo2 = 18 [(validate.rules).duration.lt = {seconds: 5}, (gogoproto.nullable) = false];
google.protobuf.Duration gogo3 = 19 [(validate.rules).duration.lt = {seconds: 5}, (gogoproto.stdduration) = true, (gogoproto.nullable) = false];
google.protobuf.Duration gogo4 = 20 [(validate.rules).duration.required = true, (gogoproto.nullable) = false];
Copy link
Member

Choose a reason for hiding this comment

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

Can these have more descriptive names? Makes it easier to debug when looking at the generator output.

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

Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

Thanks @rodaine. Can you help me bring in gogo protoc plugin into the CI docker image?

Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

Build fixed!

Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

PTAL, added docs and separate templates into go/gogo as requested.

@kyessenov kyessenov changed the title Initial gogo prototype gogo support Mar 27, 2018

{{ template "durationcmp" . }}
}
{{ end }}
Copy link
Member

Choose a reason for hiding this comment

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

The nesting here is difficult to follow. We run gofmt against generated code, so you should just structure the template for readability and let the formatter fix it in the final output.

Copy link
Member

Choose a reason for hiding this comment

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

Likewise for timestamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, responded.

Signed-off-by: Kuat Yessenov <[email protected]>
Copy link
Member

@rodaine rodaine left a comment

Choose a reason for hiding this comment

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

Thanks for getting this through!

@rodaine rodaine merged commit cae3648 into bufbuild:master Mar 27, 2018
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.

2 participants