-
Notifications
You must be signed in to change notification settings - Fork 277
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 recently added "source_file_descriptors" field of CodeGeneratorRequest #2792
Conversation
6d11262
to
f1dbb1c
Compare
…de source-only options in separate source_file_descriptors field, only for files named in file_to_generate
f1dbb1c
to
0d2c5bc
Compare
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.
An overall thing: I think we might want to make it so that we can select source-retention options or not at the ImageFile
level. A proposal for how I'd do this:
type ImageFile interface {
...
FileDescriptorProto() *descriptorpb.FileDescriptorProto
FileDescriptorProtoWithoutSourceRetentionOptions() (*descriptorpb.FileDescriptorProto, error)
...
}
Then, in imageFile
, I would do:
type imageFile struct {
...
getFileDescriptorProtoWithoutSourceRetentionOptions func() (*descriptorpb.FileDescriptorProto, error)
}
And on construction:
func newImageFileNoValidate(...) {
imageFile := &imageFile{...}
// Make sure to use syncext instead of sync as OnceValues was not in Golang <1.21.
imageFile.getFileDescriptorProtoWithoutSourceRetentionOptions =syncext.OnceValues(imageFile.getFileDescriptorProtoWithoutSourceRetentionOptionsUncached)
return imageFile
}
And at the bottom of image_file.go
, before isImageFileInfo/isImageFile
:
func (f *imageFile) getFileDescriptorProtoWithoutSourceRetentionOptionsUncached() (*descriptorpb.FileDescriptorProto, error) {
return stripSourceRetentionOptions(f.fileDescriptorProto)
}
…etention options for files to generate
… used from 'buf build'
@bufdev, this approach could expose the functionality for a Also, I tackled the TODO for more validation of the |
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.
Thanks for getting through this! Merge into bufmod
after we merge here.
I've also made sure this branch can apply to
bufmod
. Here's my tentative cherry-pick:bufmod...jh/strip-source-only-on-bufmod