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

Adding more hooks to FinishableLanguage #1443

Closed
wants to merge 9 commits into from

Conversation

linzhp
Copy link
Contributor

@linzhp linzhp commented Feb 11, 2023

What type of PR is this?
Feature

What package or component does this PR mostly affect?
cmd/gazelle

What does this PR do? Why is it needed?
DoneGeneratingRules() is called before resolving dependencies. However, some Gazelle extensions (e.g., Python) needs to start server processes to parse Python files and query standard modules before generating rules, and shutdown those processes after dependency resolution. Adding BeforeGeneratingRules and DoneResolvingDeps hooks to manage these server processes.

Other notes for review
This is a breaking change. Existing implementations of FinishableLanguage need to implement those two additional methods.

@f0rmiga
Copy link
Member

f0rmiga commented Feb 11, 2023

How about extending the hooks and having:

  • pre-generate
  • post-generate
  • post-resolve

There are other hook points that we may come up with, but those are the most obvious ones.

@linzhp
Copy link
Contributor Author

linzhp commented Feb 12, 2023

@achew22 Do you prefer a breaking change or simply move the call of DoneGeneratingRules() down?

@linzhp linzhp changed the title Calling DoneGeneratingRules after resolving deps Adding more hooks to FinishableLanguage Feb 13, 2023
@linzhp
Copy link
Contributor Author

linzhp commented Feb 13, 2023

@illicitonion Can you take another look?

@achew22
Copy link
Member

achew22 commented Feb 13, 2023

I have a strong preference for no breaking changes

@linzhp
Copy link
Contributor Author

linzhp commented Feb 13, 2023

@achew22 I see only two options to make this not break:

  1. like the original version of this PR, move the calls to DoneGeneratingRules() down, but as @illicitonion pointed out, it would change the behavior
  2. add another interface to host the two new methods. The naming would become confusing though. Now we have a FinishableLanguage interface with DoneGeneratingRules, and have another interface with BeforeGeneratingRules and DoneResolvingDeps. If we are going this route, we are sacrificing readability for backward-compatibility

Any other options?

@illicitonion
Copy link
Contributor

LGTM, thanks! I'm happy to adapt my (one) extension to whatever ends up happening, so I'm not concerned with compatibility (but understand there are compatibility concerns).

language/lang.go Outdated Show resolved Hide resolved
@linzhp linzhp mentioned this pull request Mar 14, 2023
@@ -59,8 +65,15 @@ var kinds = map[string]rule.KindInfo{
},
}

func (l *testFilegroupLang) Init(ctx context.Context) {
Copy link
Member

Choose a reason for hiding this comment

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

I thought we were going to rename this to Before?

// Init is called before any calls to GenerateRules
// This allows for hooks to be called, for instance to starting a background
// server process.
Init(ctx context.Context)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here. Am I reviewing the correct PR?

@linzhp
Copy link
Contributor Author

linzhp commented Apr 22, 2023

Superseded by #1475

@linzhp linzhp closed this Apr 22, 2023
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.

4 participants