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

re-fix configuration provider initialization #1386

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Conversation

sailingKieler
Copy link
Contributor

@sailingKieler sailingKieler commented Feb 22, 2024

This change restores the original configuration provider initialization approach contributed in #519.

To be tested:

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

The changes look good to me. @dhuebner could you test this with the grammar language?

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.

All of this starts feeling very convoluted. I think this change is a symptom of another problem in our language server logic. I'd like to present a different flow of data to improve on this and clean up the implementations a bit:

  1. Make both initialized methods async again
  2. Add ready promises to the ConfigurationProvider and WorkspaceManager interfaces. These are resolved as soon as the configuration is available and the workspace has been loaded (but not built!) respectively.
  3. Await these promises at the appropriate times (i.e. for example in the textDocument/didChange handler) to ensure that everything actually waits for the required/expected events before continuing their work.

What is your opinion on that @sailingKieler and @spoenemann? This PR in its current implementation very much feels like a workaround for design issues in our language server, instead of an actual improvement. Especially the "switch the configuration and workspace manager initialized calls around" smells like a badly designed system to me. That should never be the fix in a system that is well designed.

@sailingKieler
Copy link
Contributor Author

Especially the "switch the configuration and workspace manager initialized calls around" smells like a badly designed system to me. That should never be the fix in a system that is well designed.

Actually, the order shouldn't matter.
@spoenemann switched and fixed the invocations here, and I just flipped them back for testing purposes (and a bit because of the ConfigProvider shouldn't depend on anything, while the WorkspaceManager might depend on the former), and committed that by accident.

@msujew
Copy link
Member

msujew commented Feb 26, 2024

@spoenemann switched and fixed the invocations here

I know, and I wasn't a big fan of that either, it was just merged before I was able to complain about it :)

@spoenemann
Copy link
Contributor

Switching was not the fix, removing await was the fix. In the general case, there is no dependency between WorkspaceManager and ConfigurationProvider. Languages that need the configuration to initialize the workspace can just use ConfigurationProvider, which awaits its initialization before returning a value.

Await these promises at the appropriate times (i.e. for example in the textDocument/didChange handler) to ensure that everything actually waits for the required/expected events before continuing their work.

That sounds good to make this logical dependency more explicit.

I propose to merge this and add Mark's proposal in a follow-up PR.

@sailingKieler sailingKieler merged commit cc1a49c into main Feb 26, 2024
5 checks passed
@msujew msujew deleted the cs/configuration branch February 26, 2024 13:53
@spoenemann spoenemann added this to the v3.0.0 milestone Jun 17, 2024
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