-
Notifications
You must be signed in to change notification settings - Fork 343
Add swift_prefix to managed mode #4215
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
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| // package declaration. If the image file doesn't have a package declaration, an | ||
| // empty string is returned. | ||
| func swiftPrefixValue(imageFile bufimage.ImageFile) string { | ||
| panic("TODO") |
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.
This TODO
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.
Pushed an update for this TODO, thanks for setting this PR up!
|
Will open a PR for docs so that we have the docs ready to go when this feature is merged and released. |
| pkg := imageFile.FileDescriptorProto().GetPackage() | ||
| if pkg == "" { | ||
| return "" | ||
| } |
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.
Just confirming you checked this is right (I'm not sure if there's anything special to verify or. Ot based on what protoc's behavior is, maybe not)
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 actually had a question about this/wanted your thoughts on the behaviour.
I checked against apple/swift, and I think the following behaviour makes sense before and after this change:
- When the package is not set and
swift_prefixis not set, there will be no prefix (same before and after) - When
swift_prefixis set (in the file before the change and by managed mode after the change), the struct will get the configured prefix.
The only thing I'm unsure about is the default behaviour/what we want to set the default value for swift_prefix in managed mode to be, since swift_prefix is an optional file option for apple/swift. So when a file does not have swift_prefix at all, apple/swift will still generate code and will set the prefix of each struct to a _ delimited version of the package in pascal case, e.g.
syntax = "proto3";
package weather.acme.v1;
message Foo {...}
struct Weather_Acme_V1_Foo: ...
So I wasn't sure if we wanted to set an opinionated version of this, e.g. WeatherAcmeV1, or if we should effectively treat swift_prefix as unset when managed.enabled: true but no override is given to swift_prefix. I'm starting to think we should err on the side of the latter, just pushed up an update, but interested to get your opinion on this.
Fixes #4213.
This PR adds
swift_prefixto managed mode, e.g.If
managed.enabled: trueis set, but no override is provided, thedefault behavior for
swift_prefixis to take the pascal case of thepackage name and use a
_delimited prefix, e.g.foo.empty->Foo_Empty_.This emulates the default behavior for
apple/swiftwhenswift_prefixis not set.