-
Notifications
You must be signed in to change notification settings - Fork 273
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
Fix for protoc plugin extension reparse issue #3307
Fix for protoc plugin extension reparse issue #3307
Conversation
protoplugin.Main( | ||
protoplugin.HandlerFunc(handle), | ||
protoplugin.WithUnmarshalOptions(proto.UnmarshalOptions{ | ||
Resolver: (*protoregistry.Types)(nil), |
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 don't understand what this is doing and why - needs extensive code comments
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.
The code has been made clearer, no more typed nil.
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.
Looks good to me, will defer to @bufdev for the final stamp.
This is a solution to the bug described in #3306. Paired with protoplugin changes that allow it, this ensures that gencode shipped with buf CLI does not interfere with the reparse process by prematurely resolving extensions to the gencode versions.
(Draft until protoplugin has an appropriate interface we can use upstream. This interim version uses a
replace
directive just to demonstrate that it does indeed fix the problem.)