eclipse-langium / langium

Next-gen language engineering / DSL framework
https://langium.org/
MIT License
665 stars 61 forks source link

re-fix configuration provider initialization #1386

Closed sailingKieler closed 4 months ago

sailingKieler commented 4 months ago

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

To be tested:

sailingKieler commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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.