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

perf(lsp): cleanup workspace settings scopes #20937

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

nayeemrmn
Copy link
Collaborator

Before we were fetching and storing workspace/configuration for every opened document. But VSCode (for example) only respects differing .vscode/settings.json files per workspace folder.

This removes SpecifierSettings and simplifies our settings storage to the following:

pub struct Settings {
  pub unscoped: WorkspaceSettings,
  pub by_workspace_folder: Option<BTreeMap<ModuleSpecifier, WorkspaceSettings>>,
}

Initially, the unscoped field is populated with whatever was passed in the LSP init options. After that and on each config change event, if the client supports workspace/configuration requests, we fetch settings scoped for each workspace folder plus another 'unscoped' one to use as a fallback and to replace the stale init options.

The usage of these is abstracted under a Config::workspace_settings_for_specifier() method.

@nayeemrmn nayeemrmn requested a review from bartlomieju October 23, 2023 17:58
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM!

builder.set_root_uri(self.context.temp_dir().uri());
do_build(&mut builder);
let params: InitializeParams = builder.build();
// `config` must be updated to account for the builder changes.
// TODO(nayeemrmn): Maybe remove builder.
Copy link
Member

Choose a reason for hiding this comment

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

Why maybe remove the builder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's too many ways of supplying config, there's a separate arg for that and the value has to be saved to keep supplying it on configuration requests. The builder makes it complicated

Copy link
Member

@dsherret dsherret Oct 24, 2023

Choose a reason for hiding this comment

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

Maybe we should keep the initialization parameters and config separate? They seem to be starting to blend together here. I'm not sure though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right, yeah not the whole builder but just make the InitializeParams::initializationOptions part of it (which accepts workspace settings) into one value setter.

@nayeemrmn nayeemrmn merged commit a7bd0cf into denoland:main Oct 24, 2023
@nayeemrmn nayeemrmn deleted the lsp-workspace-settings-scopes branch October 24, 2023 22:00
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.

2 participants