-
-
Notifications
You must be signed in to change notification settings - Fork 542
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
feat(lsp): allow overriding the configuration path from LSP workspaces #5093
Conversation
@@ -492,6 +492,20 @@ impl Session { | |||
if let Some(config_path) = &self.config_path { | |||
let base_path = ConfigurationPathHint::FromUser(config_path.clone()); | |||
let status = self.load_biome_configuration_file(base_path).await; | |||
self.set_configuration_status(status); | |||
} else if let Some(config_path) = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to consider how to migrate users (LSP clients) smoothly for this change. For example, biome-intellij currently using --config-path
for overriding the config path. To prefer this new option on the plugin, we need to check the Biome version is >= 2.0. This requires some additional work for plugin side and not a smooth path.
Maybe should we prefer this new option over the existing --config-path
flag? If so, we can just put the config path to both the CLI flag and the LSP request to support Biome < 2 and >= 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are about to do a v2 release, I believe this is the right time to remove stuff. I know that users would prefer to have a deprecation warning, but I wouldn't want to carry --config-path
until we hit v3.
I believe this is the right time to have core and editor extensions to do the breaking change. VSCode extension has already removed it. We should do the same for IntelliJ and Zed.
If we remove --config-path
from lsp-proxy
, the CLI won't even start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I will rebase this branch and remove --config-path
option. Later, I will also remove from biome-intellij, and find a chance for Zed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove --config-path
in a separated PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Signed-off-by: Naoki Ikeguchi <me@s6n.jp>
37448c5
to
37de5b7
Compare
Summary
As the first step of #5089, this pull request adds a new option
experimental.configurationPath
to the response ofworkspace/configuration
request. It will affect when loading workspace configurations, preferred over the defaultbiome.json
in the workspace.Test Plan
Existing tests should pass.
For the new option, I tested my modified build of biome-intellij with
workspace/configuration
support added. I could confirm that two projects work in parallel with different Biome configurations.