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.

Sign up for GitHub

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

Resolve editor-specified config file once #15494

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+105 −52
Draft

Resolve editor-specified config file once #15494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 16 additions & 44 deletions crates/ruff_server/src/session/index/ruff_settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,6 @@ struct EditorConfigurationTransformer<'a>(&'a ResolvedEditorSettings, &'a Path);
impl ConfigurationTransformer for EditorConfigurationTransformer<'_> {
fn transform(&self, filesystem_configuration: Configuration) -> Configuration {
let ResolvedEditorSettings {
configuration,
format_preview,
lint_preview,
select,
Expand All @@ -345,17 +344,18 @@ impl ConfigurationTransformer for EditorConfigurationTransformer<'_> {
exclude,
line_length,
configuration_preference,
} = self.0.clone();

let project_root = self.1;
..
} = self.0;

let editor_configuration = Configuration {
lint: LintConfiguration {
preview: lint_preview.map(PreviewMode::from),
rule_selections: vec![RuleSelection {
select,
extend_select: extend_select.unwrap_or_default(),
ignore: ignore.unwrap_or_default(),
select: select.clone(),
extend_select: extend_select
.as_ref()
.map_or_else(Default::default, Clone::clone),
ignore: ignore.as_ref().map_or_else(Default::default, Clone::clone),
..RuleSelection::default()
}],
..LintConfiguration::default()
Expand All @@ -364,36 +364,24 @@ impl ConfigurationTransformer for EditorConfigurationTransformer<'_> {
preview: format_preview.map(PreviewMode::from),
..FormatConfiguration::default()
},
exclude: exclude.map(|exclude| {
exclude: exclude.as_ref().map(|exclude| {
exclude
.into_iter()
.iter()
.map(|pattern| {
let absolute = normalize_path_to(&pattern, project_root);
FilePattern::User(pattern, absolute)
let absolute = normalize_path_to(pattern, self.1);
FilePattern::User(pattern.clone(), absolute)
})
.collect()
}),
line_length,
line_length: *line_length,
..Configuration::default()
};

// Merge in the editor-specified configuration file, if it exists.
let editor_configuration = if let Some(config_file_path) = configuration {
tracing::debug!(
"Combining settings from editor-specified configuration file at: {}",
config_file_path.display()
);
match open_configuration_file(&config_file_path) {
Ok(config_from_file) => editor_configuration.combine(config_from_file),
err => {
tracing::error!(
"{:?}",
err.context("Unable to load editor-specified configuration file")
.unwrap_err()
);
editor_configuration
}
}
let editor_configuration = if let Some(config) = self.0.configuration() {
// TODO(dhruvmanila): This log doesn't make sense without the surrounding context
tracing::debug!("Combining settings from editor-specified configuration file");
editor_configuration.combine(config)
} else {
editor_configuration
};
Expand All @@ -409,19 +397,3 @@ impl ConfigurationTransformer for EditorConfigurationTransformer<'_> {
}
}
}

fn open_configuration_file(config_path: &Path) -> crate::Result<Configuration> {
ruff_workspace::resolver::resolve_configuration(
config_path,
Relativity::Cwd,
&IdentityTransformer,
)
}

struct IdentityTransformer;

impl ConfigurationTransformer for IdentityTransformer {
fn transform(&self, config: Configuration) -> Configuration {
config
}
}
97 changes: 89 additions & 8 deletions crates/ruff_server/src/session/settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use rustc_hash::FxHashMap;
use serde::Deserialize;

use ruff_linter::{line_width::LineLength, RuleSelector};
use ruff_workspace::configuration::Configuration;
use ruff_workspace::resolver::{resolve_configuration, ConfigurationTransformer, Relativity};

/// Maps a workspace URI to its associated client settings. Used during server initialization.
pub(crate) type WorkspaceSettingsMap = FxHashMap<Url, ClientSettings>;
Expand All @@ -13,7 +15,7 @@ pub(crate) type WorkspaceSettingsMap = FxHashMap<Url, ClientSettings>;
/// used directly by the server, and are *not* a 1:1 representation with how the client
/// sends them.
#[derive(Clone, Debug)]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq))]
#[allow(clippy::struct_excessive_bools)]
pub(crate) struct ResolvedClientSettings {
fix_all: bool,
Expand All @@ -29,9 +31,7 @@ pub(crate) struct ResolvedClientSettings {
/// LSP client settings. These fields are optional because we don't want to override file-based linter/formatting settings
/// if these were un-set.
#[derive(Clone, Debug)]
#[cfg_attr(test, derive(PartialEq, Eq))]
pub(crate) struct ResolvedEditorSettings {
pub(super) configuration: Option<PathBuf>,
pub(super) lint_preview: Option<bool>,
pub(super) format_preview: Option<bool>,
pub(super) select: Option<Vec<RuleSelector>>,
Expand All @@ -40,6 +40,58 @@ pub(crate) struct ResolvedEditorSettings {
pub(super) exclude: Option<Vec<String>>,
pub(super) line_length: Option<LineLength>,
pub(super) configuration_preference: ConfigurationPreference,

/// The resolved configuration from the editor settings, cached on first access.
resolved_configuration: Option<ResolvedConfiguration>,
}

impl ResolvedEditorSettings {
/// Returns the resolved configuration from the editor settings, if it exists.
pub(super) fn configuration(&self) -> Option<Configuration> {
self.resolved_configuration
.as_ref()?
.configuration
.clone()
.map(|config| *config)
}
}

#[derive(Clone, Debug)]
pub(crate) struct ResolvedConfiguration {
/// The path to the configuration file as specified in the editor settings.
#[allow(dead_code)]
path: PathBuf,
/// The resolved configuration from the editor settings. It will be `None` if the configuration
/// failed to load e.g., due to a file not existing or invalid configuration.
configuration: Option<Box<Configuration>>,
}

impl ResolvedConfiguration {
fn new(path: PathBuf) -> ResolvedConfiguration {
tracing::info!(
"Loading configuration from editor-specified file: {}",
path.display()
);
let configuration = match resolve_configuration(
&path,
Relativity::Cwd,
&IdentityTransformer,
) {
Ok(config) => Some(Box::new(config)),
Err(err) => {
tracing::error!("{:?}", err);
show_err_msg!(
"Failed to load editor-specified configuration file at {}. Refer to the logs for more details.",
path.display()
);
None
}
};
ResolvedConfiguration {
path,
configuration,
}
}
}

/// Determines how multiple conflicting configurations should be resolved - in this
Expand Down Expand Up @@ -305,13 +357,14 @@ impl ResolvedClientSettings {
true,
),
editor_settings: ResolvedEditorSettings {
configuration: Self::resolve_optional(all_settings, |settings| {
resolved_configuration: Self::resolve_optional(all_settings, |settings| {
settings
.configuration
.as_ref()
.and_then(|config_path| shellexpand::full(config_path).ok())
.map(|config_path| PathBuf::from(config_path.as_ref()))
}),
})
.map(ResolvedConfiguration::new),
lint_preview: Self::resolve_optional(all_settings, |settings| {
settings.lint.as_ref()?.preview
}),
Expand Down Expand Up @@ -422,6 +475,14 @@ impl Default for InitializationOptions {
}
}

struct IdentityTransformer;

impl ConfigurationTransformer for IdentityTransformer {
fn transform(&self, config: Configuration) -> Configuration {
config
}
}

#[cfg(test)]
mod tests {
use insta::assert_debug_snapshot;
Expand Down Expand Up @@ -449,6 +510,26 @@ mod tests {
serde_json::from_str(content).expect("test fixture JSON should deserialize")
}

impl PartialEq for ResolvedConfiguration {
fn eq(&self, other: &Self) -> bool {
self.path == other.path
}
}

impl PartialEq for ResolvedEditorSettings {
fn eq(&self, other: &Self) -> bool {
self.resolved_configuration == other.resolved_configuration
&& self.lint_preview == other.lint_preview
&& self.format_preview == other.format_preview
&& self.select == other.select
&& self.extend_select == other.extend_select
&& self.ignore == other.ignore
&& self.exclude == other.exclude
&& self.line_length == other.line_length
&& self.configuration_preference == other.configuration_preference
}
}

#[cfg(not(windows))]
#[test]
fn test_vs_code_init_options_deserialize() {
Expand Down Expand Up @@ -682,7 +763,7 @@ mod tests {
fix_violation_enable: false,
show_syntax_errors: true,
editor_settings: ResolvedEditorSettings {
configuration: None,
resolved_configuration: None,
lint_preview: Some(true),
format_preview: None,
select: Some(vec![
Expand Down Expand Up @@ -714,7 +795,7 @@ mod tests {
fix_violation_enable: false,
show_syntax_errors: true,
editor_settings: ResolvedEditorSettings {
configuration: None,
resolved_configuration: None,
lint_preview: Some(false),
format_preview: None,
select: Some(vec![
Expand Down Expand Up @@ -809,7 +890,7 @@ mod tests {
fix_violation_enable: true,
show_syntax_errors: true,
editor_settings: ResolvedEditorSettings {
configuration: None,
resolved_configuration: None,
lint_preview: None,
format_preview: None,
select: None,
Expand Down
Loading