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

Remove the InitializableService interface #1381

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

spoenemann
Copy link
Contributor

Repeating my comment from #1258 (comment):

Looking at it again, I'd rather remove the InitializableService interface: we already have onInitialize and onInitialized events in the LanguageServer service, which are the recommended ways to hook into the LSP lifecycle. The interface is only used by two core framework classes which require to invert the dependency. The interface is not necessary because LanguageServer hard-codes the calls to these classes anyway.

@spoenemann spoenemann added this to the v3.0.0 milestone Feb 16, 2024
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I'm in favor of this change. Looks good to me 👍

@sailingKieler
Copy link
Contributor

Actually I didn't want to pollute the service interface definitions like ConfigurationProvider and WorkspaceManager with the initialization stuff. In my customer project there's the need for more non-LSP services to be initialized with the workspace folders, so from my point of view the interface describes a standard way to implement that.

I also still have an idea in mind that is about providing an initialization hook via the constructors of DefaultConfigurationProvider and DefaultWorkspaceManager (and probably others), similar to the language server-based approach, but I didn't get to trying that, yet.

I don't have a strong opinion here.

Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

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

see comment above

@spoenemann
Copy link
Contributor Author

spoenemann commented Feb 20, 2024

In my customer project there's the need for more non-LSP services to be initialized with the workspace folders, so from my point of view the interface describes a standard way to implement that.

Do you need to separate the core part from the lsp part in that project? If not, you can simply inject LanguageServer in those non-LSP services and use the onInitialize event. IMO that is much cleaner because it puts the dependency in the right direction: from event consumer to event producer.

@spoenemann
Copy link
Contributor Author

I added a change to avoid problems during initialization: WorkspaceManager and ConfigurationProvider run their initialized hook in parallel.

@spoenemann spoenemann merged commit 1bc7009 into main Feb 22, 2024
4 checks passed
@spoenemann spoenemann deleted the remove-initializable-interface branch February 22, 2024 11:23
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.

3 participants