From 573facd2bad5d43abed7c9da903f9cf243d29d58 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 22 May 2024 11:20:45 -0700 Subject: [PATCH] Fix automatic configuration reloading for text and notebook documents (#11492) ## Summary Recent changes made in the [Jupyter Notebook feature PR](https://github.com/astral-sh/ruff/pull/11206) caused automatic configuration reloading to stop working. This was because we would check for paths to reload using the changed path, when we should have been using the parent path of the changed path (to get the directory it was changed in). Additionally, this PR fixes an issue where `ruff.toml` and `.ruff.toml` files were not being automatically reloaded. Finally, this PR improves configuration reloading by actively publishing diagnostics for notebook documents (which won't be affected by the workspace refresh since they don't use pull diagnostics). It will also publish diagnostics for text documents if pull diagnostics aren't supported. ## Test Plan To test this, open an existing configuration file in a codebase, and make modifications that will affect one or more open Python / Jupyter Notebook files. You should observe that the diagnostics for both kinds of files update automatically when the file changes are saved. Here's a test video showing what a successful test should look like: https://github.com/astral-sh/ruff/assets/19577865/7172b598-d6de-4965-b33c-6cb8b911ef6c --- crates/ruff_server/src/server.rs | 6 +++- .../notifications/did_change_watched_files.rs | 29 +++++++++++++++---- crates/ruff_server/src/session.rs | 10 +++++++ crates/ruff_server/src/session/index.rs | 23 ++++++++++++++- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/crates/ruff_server/src/server.rs b/crates/ruff_server/src/server.rs index aed36c7e01..affeeb67f0 100644 --- a/crates/ruff_server/src/server.rs +++ b/crates/ruff_server/src/server.rs @@ -173,10 +173,14 @@ impl Server { watchers: vec![ FileSystemWatcher { glob_pattern: types::GlobPattern::String( - "**/.?ruff.toml".into(), + "**/.ruff.toml".into(), ), kind: None, }, + FileSystemWatcher { + glob_pattern: types::GlobPattern::String("**/ruff.toml".into()), + kind: None, + }, FileSystemWatcher { glob_pattern: types::GlobPattern::String( "**/pyproject.toml".into(), diff --git a/crates/ruff_server/src/server/api/notifications/did_change_watched_files.rs b/crates/ruff_server/src/server/api/notifications/did_change_watched_files.rs index 5f8e79b637..15e6b60f89 100644 --- a/crates/ruff_server/src/server/api/notifications/did_change_watched_files.rs +++ b/crates/ruff_server/src/server/api/notifications/did_change_watched_files.rs @@ -1,3 +1,4 @@ +use crate::server::api::diagnostics::publish_diagnostics_for_document; use crate::server::api::LSPResult; use crate::server::client::{Notifier, Requester}; use crate::server::schedule::Task; @@ -15,7 +16,7 @@ impl super::NotificationHandler for DidChangeWatchedFiles { impl super::SyncNotificationHandler for DidChangeWatchedFiles { fn run( session: &mut Session, - _notifier: Notifier, + notifier: Notifier, requester: &mut Requester, params: types::DidChangeWatchedFilesParams, ) -> Result<()> { @@ -23,10 +24,28 @@ impl super::SyncNotificationHandler for DidChangeWatchedFiles { session.reload_settings(&change.uri.to_file_path().unwrap()); } - if session.resolved_client_capabilities().workspace_refresh && !params.changes.is_empty() { - requester - .request::((), |()| Task::nothing()) - .with_failure_code(lsp_server::ErrorCode::InternalError)?; + if !params.changes.is_empty() { + if session.resolved_client_capabilities().workspace_refresh { + requester + .request::((), |()| Task::nothing()) + .with_failure_code(lsp_server::ErrorCode::InternalError)?; + } else { + // publish diagnostics for text documents + for url in session.text_document_urls() { + let snapshot = session + .take_snapshot(&url) + .expect("snapshot should be available"); + publish_diagnostics_for_document(&snapshot, ¬ifier)?; + } + } + + // always publish diagnostics for notebook files (since they don't use pull diagnostics) + for url in session.notebook_document_urls() { + let snapshot = session + .take_snapshot(&url) + .expect("snapshot should be available"); + publish_diagnostics_for_document(&snapshot, ¬ifier)?; + } } Ok(()) diff --git a/crates/ruff_server/src/session.rs b/crates/ruff_server/src/session.rs index 3c271bb6fd..9058eed1fb 100644 --- a/crates/ruff_server/src/session.rs +++ b/crates/ruff_server/src/session.rs @@ -72,6 +72,16 @@ impl Session { }) } + /// Iterates over the LSP URLs for all open text documents. These URLs are valid file paths. + pub(super) fn text_document_urls(&self) -> impl Iterator + '_ { + self.index.text_document_urls() + } + + /// Iterates over the LSP URLs for all open notebook documents. These URLs are valid file paths. + pub(super) fn notebook_document_urls(&self) -> impl Iterator + '_ { + self.index.notebook_document_urls() + } + /// Updates a text document at the associated `key`. /// /// The document key must point to a text document, or this will throw an error. diff --git a/crates/ruff_server/src/session/index.rs b/crates/ruff_server/src/session/index.rs index 9304cba1fc..453e8eb8f2 100644 --- a/crates/ruff_server/src/session/index.rs +++ b/crates/ruff_server/src/session/index.rs @@ -88,6 +88,24 @@ impl Index { } } + pub(super) fn text_document_urls(&self) -> impl Iterator + '_ { + self.documents + .iter() + .filter(|(_, doc)| doc.as_text().is_some()) + .map(|(path, _)| { + lsp_types::Url::from_file_path(path).expect("valid file path should convert to URL") + }) + } + + pub(super) fn notebook_document_urls(&self) -> impl Iterator + '_ { + self.documents + .iter() + .filter(|(_, doc)| doc.as_notebook().is_some()) + .map(|(path, _)| { + lsp_types::Url::from_file_path(path).expect("valid file path should convert to URL") + }) + } + pub(super) fn update_text_document( &mut self, key: &DocumentKey, @@ -234,11 +252,14 @@ impl Index { Some(controller.make_ref(cell_uri, path, document_settings)) } + /// Reloads relevant existing settings files based on a changed settings file path. + /// This does not currently register new settings files. pub(super) fn reload_settings(&mut self, changed_path: &PathBuf) { + let search_path = changed_path.parent().unwrap_or(changed_path); for (root, settings) in self .settings .iter_mut() - .filter(|(path, _)| path.starts_with(changed_path)) + .filter(|(path, _)| path.starts_with(search_path)) { settings.workspace_settings_index = ruff_settings::RuffSettingsIndex::new( root,