-
-
Notifications
You must be signed in to change notification settings - Fork 237
Explicit Analyzer/Builder Overrides #3303
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
base: main
Are you sure you want to change the base?
Conversation
|
Also, right now tests are going to fail since they don't include the |
zachmu
left a comment
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.
General idea is sound, but let's avoid introducing more globals if we can help it
7830530 to
97ca0b8
Compare
zachmu
left a comment
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.
My main comment is don't put this new information in the context in order to get it to where it needs to go in non-mainline parse contexts. The catalog is a better target for that.
Otherwise I think this plan seems sound.
I think the hooks stuff is harmless enough to leave in with this batch of changes.
97ca0b8 to
843c878
Compare
zachmu
left a comment
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.
Looking a lot better, thanks for the changes.
The only additional structural change you should make is to make ExtensionHook into an interface rather than separate structs with func fields.
Related PRs: