Skip to content

Commit

Permalink
Fix automatic configuration reloading for text and notebook documents (
Browse files Browse the repository at this point in the history
…#11492)

## Summary

Recent changes made in the [Jupyter Notebook feature
PR](#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
  • Loading branch information
snowsignal committed May 22, 2024
1 parent 3cb2e67 commit 573facd
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 7 deletions.
6 changes: 5 additions & 1 deletion crates/ruff_server/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -15,18 +16,36 @@ 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<()> {
for change in &params.changes {
session.reload_settings(&change.uri.to_file_path().unwrap());
}

if session.resolved_client_capabilities().workspace_refresh && !params.changes.is_empty() {
requester
.request::<types::request::WorkspaceDiagnosticRefresh>((), |()| Task::nothing())
.with_failure_code(lsp_server::ErrorCode::InternalError)?;
if !params.changes.is_empty() {
if session.resolved_client_capabilities().workspace_refresh {
requester
.request::<types::request::WorkspaceDiagnosticRefresh>((), |()| 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, &notifier)?;
}
}

// 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, &notifier)?;
}
}

Ok(())
Expand Down
10 changes: 10 additions & 0 deletions crates/ruff_server/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Item = lsp_types::Url> + '_ {
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<Item = lsp_types::Url> + '_ {
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.
Expand Down
23 changes: 22 additions & 1 deletion crates/ruff_server/src/session/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,24 @@ impl Index {
}
}

pub(super) fn text_document_urls(&self) -> impl Iterator<Item = lsp_types::Url> + '_ {
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<Item = lsp_types::Url> + '_ {
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,
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 573facd

Please sign in to comment.