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

Allow setting protoc command via Meta #3633

Merged
merged 6 commits into from
Jan 28, 2025

Conversation

duckbrain
Copy link
Contributor

@duckbrain duckbrain commented Jan 13, 2025

Fixes #3623

Adds a meta value protoc:cmd to override the command (and prefix args).

If the meta value is provided for both the service and the API, the API will be used an API-wide default and the Service definition will be used as a Service-level override. This seems to make more sense than appending them together like we do with includes.

I wasn't sure if it made more sense to have Meta("protoc:cmd") (no values) do nothing or restore the default protoc if used as an override in the Service definition, so I made it error. Also considered not allowing it on the service definition to avoid the situation.


Tested this with a private project as well.

Copy link
Member

@raphael raphael left a comment

Choose a reason for hiding this comment

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

This looks great! I left a few suggestions - nothing major though.

grpc/codegen/proto.go Outdated Show resolved Hide resolved
dsl/meta.go Show resolved Hide resolved
grpc/codegen/proto_test.go Show resolved Hide resolved
@raphael
Copy link
Member

raphael commented Jan 24, 2025

Looks great! One tiny last modification: it would probably be good to have a header comment on the "DefaultProtoc" constant so that it is clearly documented in the dsl package online docs. Thank you!

@duckbrain
Copy link
Contributor Author

Whoops, good catch.

Thanks for the great feedback, especially pointing me to the Meta option. I started out brainstorming how to handle multiple arguments on a CLI flag.

@raphael
Copy link
Member

raphael commented Jan 28, 2025

Thank you for the great contribution!

@raphael raphael merged commit dccae52 into goadesign:v3 Jan 28, 2025
11 checks passed
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.

Allow specifying protoc command
2 participants