From 2e77b775b0e0e9fecdb6daef71c5e23fb4654e27 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 18 Jul 2024 11:05:01 +0530 Subject: [PATCH] Consider `--preview` flag for `server` subcommand (#12208) ## Summary This PR removes the requirement of `--preview` flag to run the `ruff server` and instead considers it to be an indicator to turn on preview mode for the linter and the formatter. resolves: #12161 ## Test Plan Add test cases to assert the `preview` value is updated accordingly. In an editor context, I used the local `ruff` executable in Neovim with the `--preview` flag and verified that the preview-only violations are being highlighted. Running with: ```lua require('lspconfig').ruff.setup({ cmd = { '/Users/dhruv/work/astral/ruff/target/debug/ruff', 'server', '--preview', }, }) ``` The screenshot shows that `E502` is highlighted with the below config in `pyproject.toml`: Screenshot 2024-07-17 at 16 43 09 --- crates/ruff/src/args.rs | 17 +++- crates/ruff/src/commands/server.rs | 12 +-- crates/ruff/src/lib.rs | 4 +- .../settings/empty_multiple_workspace.json | 16 ++++ .../vs_code_initialization_options.json | 27 +++--- crates/ruff_server/src/server.rs | 14 ++- crates/ruff_server/src/session/settings.rs | 92 ++++++++++++++++++- 7 files changed, 145 insertions(+), 37 deletions(-) create mode 100644 crates/ruff_server/resources/test/fixtures/settings/empty_multiple_workspace.json diff --git a/crates/ruff/src/args.rs b/crates/ruff/src/args.rs index e5fb33d812135..74448b72a745a 100644 --- a/crates/ruff/src/args.rs +++ b/crates/ruff/src/args.rs @@ -494,9 +494,20 @@ pub struct FormatCommand { #[derive(Copy, Clone, Debug, clap::Parser)] pub struct ServerCommand { - /// Enable preview mode; required for regular operation - #[arg(long)] - pub(crate) preview: bool, + /// Enable preview mode. Use `--no-preview` to disable. + /// + /// This enables unstable server features and turns on the preview mode for the linter + /// and the formatter. + #[arg(long, overrides_with("no_preview"))] + preview: bool, + #[clap(long, overrides_with("preview"), hide = true)] + no_preview: bool, +} + +impl ServerCommand { + pub(crate) fn resolve_preview(self) -> Option { + resolve_bool_arg(self.preview, self.no_preview) + } } #[derive(Debug, Clone, Copy, clap::ValueEnum)] diff --git a/crates/ruff/src/commands/server.rs b/crates/ruff/src/commands/server.rs index d35b2c1ce46f6..817269bc7e63b 100644 --- a/crates/ruff/src/commands/server.rs +++ b/crates/ruff/src/commands/server.rs @@ -4,13 +4,11 @@ use crate::ExitStatus; use anyhow::Result; use ruff_server::Server; -pub(crate) fn run_server(preview: bool, worker_threads: NonZeroUsize) -> Result { - if !preview { - tracing::error!("--preview needs to be provided as a command line argument while the server is still unstable.\nFor example: `ruff server --preview`"); - return Ok(ExitStatus::Error); - } - - let server = Server::new(worker_threads)?; +pub(crate) fn run_server( + worker_threads: NonZeroUsize, + preview: Option, +) -> Result { + let server = Server::new(worker_threads, preview)?; server.run().map(|()| ExitStatus::Success) } diff --git a/crates/ruff/src/lib.rs b/crates/ruff/src/lib.rs index 1daa634c3613c..60823478af974 100644 --- a/crates/ruff/src/lib.rs +++ b/crates/ruff/src/lib.rs @@ -200,15 +200,13 @@ fn format(args: FormatCommand, global_options: GlobalConfigArgs) -> Result Result { - let ServerCommand { preview } = args; - let four = NonZeroUsize::new(4).unwrap(); // by default, we set the number of worker threads to `num_cpus`, with a maximum of 4. let worker_threads = std::thread::available_parallelism() .unwrap_or(four) .max(four); - commands::server::run_server(preview, worker_threads) + commands::server::run_server(worker_threads, args.resolve_preview()) } pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result { diff --git a/crates/ruff_server/resources/test/fixtures/settings/empty_multiple_workspace.json b/crates/ruff_server/resources/test/fixtures/settings/empty_multiple_workspace.json new file mode 100644 index 0000000000000..ec6cc3f9fca5b --- /dev/null +++ b/crates/ruff_server/resources/test/fixtures/settings/empty_multiple_workspace.json @@ -0,0 +1,16 @@ +{ + "settings": [ + { + "cwd": "/Users/test/projects/first", + "workspace": "file:///Users/test/projects/first" + }, + { + "cwd": "/Users/test/projects/second", + "workspace": "file:///Users/test/projects/second" + } + ], + "globalSettings": { + "cwd": "/", + "workspace": "/" + } +} diff --git a/crates/ruff_server/resources/test/fixtures/settings/vs_code_initialization_options.json b/crates/ruff_server/resources/test/fixtures/settings/vs_code_initialization_options.json index 7ce732add22b2..e6b6ccf5978f6 100644 --- a/crates/ruff_server/resources/test/fixtures/settings/vs_code_initialization_options.json +++ b/crates/ruff_server/resources/test/fixtures/settings/vs_code_initialization_options.json @@ -1,7 +1,7 @@ { "settings": [ { - "experimentalServer": true, + "nativeServer": "on", "cwd": "/Users/test/projects/pandas", "workspace": "file:///Users/test/projects/pandas", "path": [], @@ -21,9 +21,7 @@ "lint": { "enable": true, "run": "onType", - "args": [ - "--preview" - ] + "args": [] }, "format": { "args": [] @@ -31,10 +29,11 @@ "enable": true, "organizeImports": true, "fixAll": true, - "showNotifications": "off" + "showNotifications": "off", + "showSyntaxErrors": true }, { - "experimentalServer": true, + "nativeServer": "on", "cwd": "/Users/test/projects/scipy", "workspace": "file:///Users/test/projects/scipy", "path": [], @@ -55,9 +54,7 @@ "enable": true, "preview": false, "run": "onType", - "args": [ - "--preview" - ] + "args": [] }, "format": { "args": [] @@ -65,11 +62,12 @@ "enable": true, "organizeImports": true, "fixAll": true, - "showNotifications": "off" + "showNotifications": "off", + "showSyntaxErrors": true } ], "globalSettings": { - "experimentalServer": true, + "nativeServer": "on", "cwd": "/", "workspace": "/", "path": [], @@ -89,9 +87,7 @@ "preview": true, "select": ["F", "I"], "run": "onType", - "args": [ - "--preview" - ] + "args": [] }, "format": { "args": [] @@ -99,6 +95,7 @@ "enable": true, "organizeImports": true, "fixAll": false, - "showNotifications": "off" + "showNotifications": "off", + "showSyntaxErrors": true } } diff --git a/crates/ruff_server/src/server.rs b/crates/ruff_server/src/server.rs index 1a778e8c00a94..03f52175a7332 100644 --- a/crates/ruff_server/src/server.rs +++ b/crates/ruff_server/src/server.rs @@ -48,7 +48,7 @@ pub struct Server { } impl Server { - pub fn new(worker_threads: NonZeroUsize) -> crate::Result { + pub fn new(worker_threads: NonZeroUsize, preview: Option) -> crate::Result { let connection = ConnectionInitializer::stdio(); let (id, init_params) = connection.initialize_start()?; @@ -70,14 +70,18 @@ impl Server { crate::message::init_messenger(connection.make_sender()); - let AllSettings { - global_settings, - mut workspace_settings, - } = AllSettings::from_value( + let mut all_settings = AllSettings::from_value( init_params .initialization_options .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::default())), ); + if let Some(preview) = preview { + all_settings.set_preview(preview); + } + let AllSettings { + global_settings, + mut workspace_settings, + } = all_settings; crate::trace::init_tracing( connection.make_sender(), diff --git a/crates/ruff_server/src/session/settings.rs b/crates/ruff_server/src/session/settings.rs index 3ec0a04c2fe85..10616fb2eda6c 100644 --- a/crates/ruff_server/src/session/settings.rs +++ b/crates/ruff_server/src/session/settings.rs @@ -84,6 +84,20 @@ pub struct ClientSettings { pub(crate) tracing: TracingSettings, } +impl ClientSettings { + /// Update the preview flag for the linter and the formatter with the given value. + pub(crate) fn set_preview(&mut self, preview: bool) { + match self.lint.as_mut() { + None => self.lint = Some(LintOptions::default().with_preview(preview)), + Some(lint) => lint.set_preview(preview), + } + match self.format.as_mut() { + None => self.format = Some(FormatOptions::default().with_preview(preview)), + Some(format) => format.set_preview(preview), + } + } +} + /// Settings needed to initialize tracing. These will only be /// read from the global configuration. #[derive(Debug, Deserialize, Default)] @@ -107,7 +121,7 @@ struct WorkspaceSettings { workspace: Url, } -#[derive(Debug, Deserialize)] +#[derive(Debug, Default, Deserialize)] #[cfg_attr(test, derive(PartialEq, Eq))] #[serde(rename_all = "camelCase")] struct LintOptions { @@ -118,6 +132,17 @@ struct LintOptions { ignore: Option>, } +impl LintOptions { + fn with_preview(mut self, preview: bool) -> LintOptions { + self.preview = Some(preview); + self + } + + fn set_preview(&mut self, preview: bool) { + self.preview = Some(preview); + } +} + #[derive(Debug, Default, Deserialize)] #[cfg_attr(test, derive(PartialEq, Eq))] #[serde(rename_all = "camelCase")] @@ -125,6 +150,17 @@ struct FormatOptions { preview: Option, } +impl FormatOptions { + fn with_preview(mut self, preview: bool) -> FormatOptions { + self.preview = Some(preview); + self + } + + fn set_preview(&mut self, preview: bool) { + self.preview = Some(preview); + } +} + #[derive(Debug, Default, Deserialize)] #[cfg_attr(test, derive(PartialEq, Eq))] #[serde(rename_all = "camelCase")] @@ -159,6 +195,7 @@ enum InitializationOptions { } /// Built from the initialization options provided by the client. +#[derive(Debug)] pub(crate) struct AllSettings { pub(crate) global_settings: ClientSettings, /// If this is `None`, the client only passed in global settings. @@ -179,6 +216,16 @@ impl AllSettings { ) } + /// Update the preview flag for both the global and all workspace settings. + pub(crate) fn set_preview(&mut self, preview: bool) { + self.global_settings.set_preview(preview); + if let Some(workspace_settings) = self.workspace_settings.as_mut() { + for settings in workspace_settings.values_mut() { + settings.set_preview(preview); + } + } + } + fn from_init_options(options: InitializationOptions) -> Self { let (global_settings, workspace_settings) = match options { InitializationOptions::GlobalOnly { settings } => (settings, None), @@ -393,6 +440,11 @@ mod tests { const EMPTY_INIT_OPTIONS_FIXTURE: &str = include_str!("../../resources/test/fixtures/settings/empty.json"); + // This fixture contains multiple workspaces with empty initialization options. It only sets + // the `cwd` and the `workspace` value. + const EMPTY_MULTIPLE_WORKSPACE_INIT_OPTIONS_FIXTURE: &str = + include_str!("../../resources/test/fixtures/settings/empty_multiple_workspace.json"); + fn deserialize_fixture(content: &str) -> T { serde_json::from_str(content).expect("test fixture JSON should deserialize") } @@ -456,7 +508,9 @@ mod tests { exclude: None, line_length: None, configuration_preference: None, - show_syntax_errors: None, + show_syntax_errors: Some( + true, + ), tracing: TracingSettings { log_level: None, log_file: None, @@ -509,7 +563,9 @@ mod tests { exclude: None, line_length: None, configuration_preference: None, - show_syntax_errors: None, + show_syntax_errors: Some( + true, + ), tracing: TracingSettings { log_level: None, log_file: None, @@ -575,7 +631,9 @@ mod tests { exclude: None, line_length: None, configuration_preference: None, - show_syntax_errors: None, + show_syntax_errors: Some( + true, + ), tracing: TracingSettings { log_level: None, log_file: None, @@ -771,4 +829,30 @@ mod tests { assert_eq!(options, InitializationOptions::default()); } + + fn assert_preview_client_settings(settings: &ClientSettings, preview: bool) { + assert_eq!(settings.lint.as_ref().unwrap().preview.unwrap(), preview); + assert_eq!(settings.format.as_ref().unwrap().preview.unwrap(), preview); + } + + fn assert_preview_all_settings(all_settings: &AllSettings, preview: bool) { + assert_preview_client_settings(&all_settings.global_settings, preview); + if let Some(workspace_settings) = all_settings.workspace_settings.as_ref() { + for settings in workspace_settings.values() { + assert_preview_client_settings(settings, preview); + } + } + } + + #[test] + fn test_preview_flag() { + let options = deserialize_fixture(EMPTY_MULTIPLE_WORKSPACE_INIT_OPTIONS_FIXTURE); + let mut all_settings = AllSettings::from_init_options(options); + + all_settings.set_preview(false); + assert_preview_all_settings(&all_settings, false); + + all_settings.set_preview(true); + assert_preview_all_settings(&all_settings, true); + } }