-
Notifications
You must be signed in to change notification settings - Fork 2k
Expand ruff.configuration to allow inline config
#16296
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
Changes from 5 commits
3417a97
cda5a52
b6e3adf
db651b9
c60924d
ff5a7a1
cb50bd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| { | ||
| "settings": { | ||
| "configuration": { | ||
| "line-length": 100, | ||
| "lint": { | ||
| "extend-select": ["I001"] | ||
| }, | ||
| "format": { | ||
| "quote-style": "single" | ||
| } | ||
| }, | ||
| "lint": { | ||
| "extendSelect": ["RUF001"] | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3,8 +3,11 @@ use std::{ops::Deref, path::PathBuf, str::FromStr}; | |||
| use lsp_types::Url; | ||||
| use rustc_hash::FxHashMap; | ||||
| use serde::Deserialize; | ||||
| use serde_json::{Map, Value}; | ||||
| use thiserror::Error; | ||||
|
|
||||
| use ruff_linter::{line_width::LineLength, RuleSelector}; | ||||
| use ruff_workspace::options::Options; | ||||
|
|
||||
| /// Maps a workspace URI to its associated client settings. Used during server initialization. | ||||
| pub(crate) type WorkspaceSettingsMap = FxHashMap<Url, ClientSettings>; | ||||
|
|
@@ -29,9 +32,9 @@ 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))] | ||||
| #[cfg_attr(test, derive(Default, PartialEq, Eq))] | ||||
| pub(crate) struct ResolvedEditorSettings { | ||||
| pub(super) configuration: Option<PathBuf>, | ||||
| pub(super) configuration: Option<ResolvedConfiguration>, | ||||
| pub(super) lint_preview: Option<bool>, | ||||
| pub(super) format_preview: Option<bool>, | ||||
| pub(super) select: Option<Vec<RuleSelector>>, | ||||
|
|
@@ -42,6 +45,48 @@ pub(crate) struct ResolvedEditorSettings { | |||
| pub(super) configuration_preference: ConfigurationPreference, | ||||
| } | ||||
|
|
||||
| /// The resolved configuration from the client settings. | ||||
| #[derive(Clone, Debug)] | ||||
| #[cfg_attr(test, derive(PartialEq, Eq))] | ||||
| pub(crate) enum ResolvedConfiguration { | ||||
| FilePath(PathBuf), | ||||
| Inline(Options), | ||||
| } | ||||
|
|
||||
| impl TryFrom<&ClientConfiguration> for ResolvedConfiguration { | ||||
| type Error = ResolvedConfigurationError; | ||||
|
|
||||
| fn try_from(value: &ClientConfiguration) -> Result<Self, Self::Error> { | ||||
| match value { | ||||
| ClientConfiguration::String(path) => Ok(ResolvedConfiguration::FilePath( | ||||
| PathBuf::from(shellexpand::full(path)?.as_ref()), | ||||
| )), | ||||
| ClientConfiguration::Object(map) => { | ||||
| let options = toml::Table::try_from(map)?.try_into::<Options>()?; | ||||
| if options.extend.is_some() { | ||||
| Err(ResolvedConfigurationError::ExtendNotSupported) | ||||
| } else { | ||||
| Ok(ResolvedConfiguration::Inline(options)) | ||||
| } | ||||
| } | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| /// An error that can occur when trying to resolve the `configuration` value from the client | ||||
| /// settings. | ||||
| #[derive(Debug, Error)] | ||||
| pub(crate) enum ResolvedConfigurationError { | ||||
| #[error(transparent)] | ||||
| EnvVarLookupError(#[from] shellexpand::LookupError<std::env::VarError>), | ||||
| #[error("error serializing configuration to TOML: {0}")] | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unclear to me where we serialize the value to TOML. I understand deserialization but I don't see where we perform any serialization. I worry that this error message is also not very helpful for users because it's not clear what's going wrong. Can we use a more specific error message? Failed to load configuration from
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The serialization of
I'll update the error message
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to keep this message for now. The "Failed to load ..." message that you've suggested is already going to be appended before this. The reason I want to keep this is that for something like: "ruff.configuration": {
"line-length": null
},We'll get: But, with the above message: We'll atleast know that the issue is with TOML deserialization
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think I would understand this message or even know what I have to do.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think one solution would be to handle these
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could skip the intermediate JSON deserialization but that would yield to even worse experience: |
||||
| InvalidToml(#[from] toml::ser::Error), | ||||
| #[error(transparent)] | ||||
| InvalidRuffSchema(#[from] toml::de::Error), | ||||
| #[error("using `extend` is unsupported for inline configuration")] | ||||
|
dhruvmanila marked this conversation as resolved.
|
||||
| ExtendNotSupported, | ||||
| } | ||||
|
|
||||
| /// Determines how multiple conflicting configurations should be resolved - in this | ||||
| /// case, the configuration from the client settings and configuration from local | ||||
| /// `.toml` files (aka 'workspace' configuration). | ||||
|
|
@@ -57,12 +102,23 @@ pub(crate) enum ConfigurationPreference { | |||
| EditorOnly, | ||||
| } | ||||
|
|
||||
| /// A direct representation of of `configuration` schema within the client settings. | ||||
| #[derive(Debug, Deserialize)] | ||||
| #[cfg_attr(test, derive(PartialEq, Eq))] | ||||
| #[serde(untagged)] | ||||
| enum ClientConfiguration { | ||||
| /// A path to a configuration file. | ||||
| String(String), | ||||
| /// An object containing the configuration options. | ||||
| Object(Map<String, Value>), | ||||
| } | ||||
|
|
||||
| /// This is a direct representation of the settings schema sent by the client. | ||||
| #[derive(Debug, Deserialize, Default)] | ||||
| #[cfg_attr(test, derive(PartialEq, Eq))] | ||||
| #[serde(rename_all = "camelCase")] | ||||
| pub struct ClientSettings { | ||||
| configuration: Option<String>, | ||||
| configuration: Option<ClientConfiguration>, | ||||
| fix_all: Option<bool>, | ||||
| organize_imports: Option<bool>, | ||||
| lint: Option<LintOptions>, | ||||
|
|
@@ -306,11 +362,15 @@ impl ResolvedClientSettings { | |||
| ), | ||||
| editor_settings: ResolvedEditorSettings { | ||||
| 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())) | ||||
| settings.configuration.as_ref().and_then(|configuration| { | ||||
| match ResolvedConfiguration::try_from(configuration) { | ||||
| Ok(configuration) => Some(configuration), | ||||
| Err(err) => { | ||||
| tracing::error!("Failed to resolve configuration: {err}"); | ||||
|
dhruvmanila marked this conversation as resolved.
Outdated
|
||||
| None | ||||
| } | ||||
| } | ||||
| }) | ||||
|
Comment on lines
+359
to
+375
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm planning to open a follow-up PR to notify the user that resolving the client settings failed. We currently don't log or notify the user if it fails. For example, |
||||
| }), | ||||
| lint_preview: Self::resolve_optional(all_settings, |settings| { | ||||
| settings.lint.as_ref()?.preview | ||||
|
|
@@ -425,6 +485,10 @@ impl Default for InitializationOptions { | |||
| #[cfg(test)] | ||||
| mod tests { | ||||
| use insta::assert_debug_snapshot; | ||||
| use ruff_python_formatter::QuoteStyle; | ||||
| use ruff_workspace::options::{ | ||||
| FormatOptions as RuffFormatOptions, LintCommonOptions, LintOptions, | ||||
| }; | ||||
| use serde::de::DeserializeOwned; | ||||
|
|
||||
| #[cfg(not(windows))] | ||||
|
|
@@ -445,6 +509,9 @@ mod tests { | |||
| const EMPTY_MULTIPLE_WORKSPACE_INIT_OPTIONS_FIXTURE: &str = | ||||
| include_str!("../../resources/test/fixtures/settings/empty_multiple_workspace.json"); | ||||
|
|
||||
| const INLINE_CONFIGURATION_FIXTURE: &str = | ||||
| include_str!("../../resources/test/fixtures/settings/inline_configuration.json"); | ||||
|
|
||||
| fn deserialize_fixture<T: DeserializeOwned>(content: &str) -> T { | ||||
| serde_json::from_str(content).expect("test fixture JSON should deserialize") | ||||
| } | ||||
|
|
@@ -855,4 +922,48 @@ mod tests { | |||
| all_settings.set_preview(true); | ||||
| assert_preview_all_settings(&all_settings, true); | ||||
| } | ||||
|
|
||||
| #[test] | ||||
| fn inline_configuration() { | ||||
| let options: InitializationOptions = deserialize_fixture(INLINE_CONFIGURATION_FIXTURE); | ||||
|
|
||||
| let AllSettings { | ||||
| global_settings, | ||||
| workspace_settings: None, | ||||
| } = AllSettings::from_init_options(options) | ||||
| else { | ||||
| panic!("Expected global settings only"); | ||||
| }; | ||||
|
|
||||
| assert_eq!( | ||||
| ResolvedClientSettings::global(&global_settings), | ||||
| ResolvedClientSettings { | ||||
| fix_all: true, | ||||
| organize_imports: true, | ||||
| lint_enable: true, | ||||
| disable_rule_comment_enable: true, | ||||
| fix_violation_enable: true, | ||||
| show_syntax_errors: true, | ||||
| editor_settings: ResolvedEditorSettings { | ||||
| configuration: Some(ResolvedConfiguration::Inline(Options { | ||||
| line_length: Some(LineLength::try_from(100).unwrap()), | ||||
| lint: Some(LintOptions { | ||||
| common: LintCommonOptions { | ||||
| extend_select: Some(vec![RuleSelector::from_str("I001").unwrap()]), | ||||
| ..Default::default() | ||||
| }, | ||||
| ..Default::default() | ||||
| }), | ||||
| format: Some(RuffFormatOptions { | ||||
| quote_style: Some(QuoteStyle::Single), | ||||
| ..Default::default() | ||||
| }), | ||||
| ..Default::default() | ||||
| })), | ||||
| extend_select: Some(vec![RuleSelector::from_str("RUF001").unwrap()]), | ||||
| ..Default::default() | ||||
| } | ||||
| } | ||||
| ); | ||||
| } | ||||
| } | ||||
Uh oh!
There was an error while loading. Please reload this page.