Skip to content

feat: allow configuration through workspace/didChangeConfiguration #316

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/pgt_configuration/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub use analyser::{
RulePlainConfiguration, RuleSelector, RuleWithFixOptions, RuleWithOptions, Rules,
partial_linter_configuration,
};
use biome_deserialize::Merge;
use biome_deserialize_macros::{Merge, Partial};
use bpaf::Bpaf;
use database::{
Expand Down Expand Up @@ -115,6 +116,11 @@ impl PartialConfiguration {
}),
}
}

pub fn merge(&mut self, other: Self) -> Self {
self.merge_with(other);
self.clone()
}
Comment on lines +120 to +123
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you do what I wrote above, we don't need this helper and the clone 👍🏻

}

pub struct ConfigurationPayload {
Expand Down
10 changes: 6 additions & 4 deletions crates/pgt_lsp/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ impl LanguageServer for LSPServer {
ConfigName::pgt_jsonc()
);

futures::join!(self.session.load_workspace_settings());
futures::join!(self.session.load_workspace_settings(None));

let msg = format!("Server initialized with PID: {}", std::process::id());
self.session
Expand All @@ -152,8 +152,10 @@ impl LanguageServer for LSPServer {
}

#[tracing::instrument(level = "info", skip_all)]
async fn did_change_configuration(&self, _params: DidChangeConfigurationParams) {
self.session.load_workspace_settings().await;
async fn did_change_configuration(&self, params: DidChangeConfigurationParams) {
self.session
.load_workspace_settings(serde_json::from_value(params.settings).ok())
.await;
self.setup_capabilities().await;
self.session.update_all_diagnostics().await;
}
Expand All @@ -174,7 +176,7 @@ impl LanguageServer for LSPServer {
if ConfigName::file_names()
.contains(&&*watched_file.display().to_string())
{
self.session.load_workspace_settings().await;
self.session.load_workspace_settings(None).await;
self.setup_capabilities().await;
// self.session.update_all_diagnostics().await;
// for now we are only interested to the configuration file,
Expand Down
21 changes: 13 additions & 8 deletions crates/pgt_lsp/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use anyhow::Result;
use futures::StreamExt;
use futures::stream::FuturesUnordered;
use pgt_analyse::RuleCategoriesBuilder;
use pgt_configuration::ConfigurationPathHint;
use pgt_configuration::{ConfigurationPathHint, PartialConfiguration};
use pgt_diagnostics::{DiagnosticExt, Error};
use pgt_fs::{FileSystem, PgTPath};
use pgt_workspace::Workspace;
Expand Down Expand Up @@ -386,11 +386,11 @@ impl Session {
/// This function attempts to read the `postgrestools.jsonc` configuration file from
/// the root URI and update the workspace settings accordingly
#[tracing::instrument(level = "trace", skip(self))]
pub(crate) async fn load_workspace_settings(&self) {
pub(crate) async fn load_workspace_settings(&self, params: Option<PartialConfiguration>) {
Copy link
Collaborator

@juleswritescode juleswritescode Apr 11, 2025

Choose a reason for hiding this comment

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

I liked the extra_config name – would you mind renaming this as well?

// Providing a custom configuration path will not allow to support workspaces
if let Some(config_path) = &self.config_path {
let base_path = ConfigurationPathHint::FromUser(config_path.clone());
let status = self.load_pgt_configuration_file(base_path).await;
let status = self.load_pgt_configuration_file(base_path, params).await;
self.set_configuration_status(status);
} else if let Some(folders) = self.get_workspace_folders() {
info!("Detected workspace folder.");
Expand All @@ -401,9 +401,10 @@ impl Session {
match base_path {
Ok(base_path) => {
let status = self
.load_pgt_configuration_file(ConfigurationPathHint::FromWorkspace(
base_path,
))
.load_pgt_configuration_file(
ConfigurationPathHint::FromWorkspace(base_path),
params.clone(),
)
.await;
self.set_configuration_status(status);
}
Expand All @@ -420,14 +421,15 @@ impl Session {
None => ConfigurationPathHint::default(),
Some(path) => ConfigurationPathHint::FromLsp(path),
};
let status = self.load_pgt_configuration_file(base_path).await;
let status = self.load_pgt_configuration_file(base_path, params).await;
self.set_configuration_status(status);
}
}

async fn load_pgt_configuration_file(
&self,
base_path: ConfigurationPathHint,
extra_config: Option<PartialConfiguration>,
) -> ConfigurationStatus {
match load_configuration(&self.fs, base_path.clone()) {
Ok(loaded_configuration) => {
Expand All @@ -446,7 +448,10 @@ impl Session {
Ok((vcs_base_path, gitignore_matches)) => {
let result = self.workspace.update_settings(UpdateSettingsParams {
workspace_directory: self.fs.working_directory(),
configuration: fs_configuration,
configuration: match extra_config {
Some(config) => fs_configuration.clone().merge(config),
None => fs_configuration,
},
Comment on lines +451 to +454
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need the extra allocation (caused by clone()) – plus, if the extra_config has any gitignore related settings, these would be ignored.

Maybe it is better to do a fs_configuration.merge_with(extra_config) a few lines above (if extra_config is Some(..))?

If you use the .merge_with method from the biome Merge trait, you probably don't even need an extra method 👍

Let me know if you need help with that.

vcs_base_path,
gitignore_matches,
skip_db: false,
Expand Down
1 change: 1 addition & 0 deletions crates/pgt_workspace/src/workspace/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ impl Workspace for WorkspaceServer {
)?;

tracing::info!("Updated settings in workspace");
tracing::debug!("Updated settings are {:#?}", self.settings());

if !params.skip_db {
self.connection
Expand Down