From dc411bf7ad5ed8b8aad89d2241cf71b789047551 Mon Sep 17 00:00:00 2001 From: l3ops Date: Fri, 9 Dec 2022 11:49:28 +0100 Subject: [PATCH 1/5] fix(rome_service): improve the pattern matching logic --- crates/rome_service/src/matcher/pattern.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/rome_service/src/matcher/pattern.rs b/crates/rome_service/src/matcher/pattern.rs index 9a9b0fe028a..c813e474fa9 100644 --- a/crates/rome_service/src/matcher/pattern.rs +++ b/crates/rome_service/src/matcher/pattern.rs @@ -122,6 +122,27 @@ impl Pattern { let mut is_recursive = false; let mut i = 0; + // A pattern is relative if it starts with "." followed by a separator + let is_relative = matches!(chars.get(..2), Some(['.', sep]) if path::is_separator(*sep)); + if is_relative { + // If a pattern starts with a relative prefix, strip it from the pattern + i += 2; + } else { + // A pattern is absolute if it starts with a path separator + let mut is_absolute = chars.first().map_or(false, |c| path::is_separator(*c)); + + // On windows a pattern may also be absolute if it starts with a drive letter, a colon and a separator + if cfg!(windows) && !is_absolute { + is_absolute = matches!(chars.get(..3), Some(['a'..='z' | 'A'..='Z', ':', sep]) if path::is_separator(*sep)); + } + + // If a pattern is not absolute, insert a "**/" sequence in front + if !is_absolute { + tokens.push(AnyRecursiveSequence); + tokens.push(Char('/')); + } + } + while i < chars.len() { match chars[i] { '?' => { From b159e38a5598d884047407acbb12336431a3335a Mon Sep 17 00:00:00 2001 From: l3ops Date: Fri, 9 Dec 2022 15:58:29 +0100 Subject: [PATCH 2/5] strip the workspace root on paths in the language server --- crates/rome_lsp/src/handlers/analysis.rs | 4 +-- crates/rome_lsp/src/handlers/formatting.rs | 6 ++--- crates/rome_lsp/src/handlers/rename.rs | 2 +- crates/rome_lsp/src/handlers/text_document.rs | 6 ++--- crates/rome_lsp/src/requests/syntax_tree.rs | 2 +- crates/rome_lsp/src/session.rs | 25 ++++++++++++++++--- 6 files changed, 31 insertions(+), 14 deletions(-) diff --git a/crates/rome_lsp/src/handlers/analysis.rs b/crates/rome_lsp/src/handlers/analysis.rs index bd9cd50a4cd..77e18aa72cc 100644 --- a/crates/rome_lsp/src/handlers/analysis.rs +++ b/crates/rome_lsp/src/handlers/analysis.rs @@ -32,7 +32,7 @@ pub(crate) fn code_actions( params: CodeActionParams, ) -> Result> { let url = params.text_document.uri.clone(); - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; let unsupported_lint = &session.workspace.supports_feature(SupportsFeatureParams { path: rome_path, @@ -57,7 +57,7 @@ pub(crate) fn code_actions( } let url = params.text_document.uri.clone(); - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; let doc = session.document(&url)?; let diagnostics = params.context.diagnostics; diff --git a/crates/rome_lsp/src/handlers/formatting.rs b/crates/rome_lsp/src/handlers/formatting.rs index e88180c2eff..766bc325484 100644 --- a/crates/rome_lsp/src/handlers/formatting.rs +++ b/crates/rome_lsp/src/handlers/formatting.rs @@ -15,7 +15,7 @@ pub(crate) fn format( params: DocumentFormattingParams, ) -> Result>> { let url = params.text_document.uri; - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; let doc = session.document(&url)?; @@ -56,7 +56,7 @@ pub(crate) fn format_range( params: DocumentRangeFormattingParams, ) -> Result>> { let url = params.text_document.uri; - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; let doc = session.document(&url)?; let start_index = utils::offset(&doc.line_index, params.range.start).with_context(|| { @@ -119,7 +119,7 @@ pub(crate) fn format_on_type( let url = params.text_document_position.text_document.uri; let position = params.text_document_position.position; - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; let doc = session.document(&url)?; let offset = utils::offset(&doc.line_index, position) diff --git a/crates/rome_lsp/src/handlers/rename.rs b/crates/rome_lsp/src/handlers/rename.rs index 4314f245173..2a625f3234c 100644 --- a/crates/rome_lsp/src/handlers/rename.rs +++ b/crates/rome_lsp/src/handlers/rename.rs @@ -8,7 +8,7 @@ use tracing::trace; #[tracing::instrument(level = "debug", skip(session), err)] pub(crate) fn rename(session: &Session, params: RenameParams) -> Result> { let url = params.text_document_position.text_document.uri; - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; trace!("Renaming..."); diff --git a/crates/rome_lsp/src/handlers/text_document.rs b/crates/rome_lsp/src/handlers/text_document.rs index 61b6a6cb0e6..9b6e1f72e95 100644 --- a/crates/rome_lsp/src/handlers/text_document.rs +++ b/crates/rome_lsp/src/handlers/text_document.rs @@ -18,7 +18,7 @@ pub(crate) async fn did_open( let content = params.text_document.text; let language_hint = Language::from_language_id(¶ms.text_document.language_id); - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; let doc = Document::new(version, &content); session.workspace.open_file(OpenFileParams { @@ -46,7 +46,7 @@ pub(crate) async fn did_change( let url = params.text_document.uri; let version = params.text_document.version; - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; let doc = session.document(&url)?; let mut content = doc.content; @@ -93,7 +93,7 @@ pub(crate) async fn did_close( params: lsp_types::DidCloseTextDocumentParams, ) -> Result<()> { let url = params.text_document.uri; - let rome_path = session.file_path(&url); + let rome_path = session.file_path(&url)?; session .workspace diff --git a/crates/rome_lsp/src/requests/syntax_tree.rs b/crates/rome_lsp/src/requests/syntax_tree.rs index 66fafb072cb..9484492c718 100644 --- a/crates/rome_lsp/src/requests/syntax_tree.rs +++ b/crates/rome_lsp/src/requests/syntax_tree.rs @@ -15,7 +15,7 @@ pub struct SyntaxTreePayload { pub(crate) fn syntax_tree(session: &Session, url: &Url) -> Result { info!("Showing syntax tree"); - let rome_path = session.file_path(url); + let rome_path = session.file_path(url)?; let syntax_tree = session .workspace .get_syntax_tree(GetSyntaxTreeParams { path: rome_path })?; diff --git a/crates/rome_lsp/src/session.rs b/crates/rome_lsp/src/session.rs index f8b1f40d2b5..91157e0a1f8 100644 --- a/crates/rome_lsp/src/session.rs +++ b/crates/rome_lsp/src/session.rs @@ -3,6 +3,7 @@ use crate::config::CONFIGURATION_SECTION; use crate::documents::Document; use crate::url_interner::UrlInterner; use crate::utils; +use anyhow::{anyhow, Result}; use futures::stream::futures_unordered::FuturesUnordered; use futures::StreamExt; use rome_analyze::RuleCategories; @@ -127,17 +128,33 @@ impl Session { self.url_interner.write().unwrap().intern(url) } - pub(crate) fn file_path(&self, url: &lsp_types::Url) -> RomePath { + pub(crate) fn file_path(&self, url: &lsp_types::Url) -> Result { let file_id = self.file_id(url.clone()); - RomePath::new(url.path(), file_id) + let mut path_to_file = url + .to_file_path() + .map_err(|()| anyhow!("failed to convert {url} to a filesystem path"))?; + + let relative_path = { + let root_uri = self.root_uri.read().unwrap(); + root_uri.as_ref().and_then(|root_uri| { + let root_path = root_uri.to_file_path().ok()?; + path_to_file.strip_prefix(&root_path).ok() + }) + }; + + if let Some(relative_path) = relative_path { + path_to_file = relative_path.into(); + } + + Ok(RomePath::new(path_to_file, file_id)) } /// Computes diagnostics for the file matching the provided url and publishes /// them to the client. Called from [`handlers::text_document`] when a file's /// contents changes. #[tracing::instrument(level = "debug", skip_all, fields(url = display(&url), diagnostic_count), err)] - pub(crate) async fn update_diagnostics(&self, url: lsp_types::Url) -> anyhow::Result<()> { - let rome_path = self.file_path(&url); + pub(crate) async fn update_diagnostics(&self, url: lsp_types::Url) -> Result<()> { + let rome_path = self.file_path(&url)?; let doc = self.document(&url)?; let unsupported_lint = self.workspace.supports_feature(SupportsFeatureParams { feature: FeatureName::Lint, From e98cd71cf00783815c808c25a6990ffefa38f33a Mon Sep 17 00:00:00 2001 From: l3ops Date: Fri, 9 Dec 2022 17:18:52 +0100 Subject: [PATCH 3/5] update tests --- crates/rome_cli/tests/commands/format.rs | 53 +++++++++++-------- crates/rome_cli/tests/configs.rs | 8 ++- .../does_not_format_ignored_directories.snap | 39 ++++++++++++-- crates/rome_lsp/tests/server.rs | 47 +++++++++------- crates/rome_service/src/matcher/pattern.rs | 6 +-- 5 files changed, 104 insertions(+), 49 deletions(-) diff --git a/crates/rome_cli/tests/commands/format.rs b/crates/rome_cli/tests/commands/format.rs index 24f40384eb7..36a3a55a382 100644 --- a/crates/rome_cli/tests/commands/format.rs +++ b/crates/rome_cli/tests/commands/format.rs @@ -1104,17 +1104,26 @@ fn does_not_format_if_files_are_listed_in_ignore_option() { fn does_not_format_ignored_directories() { let mut console = BufferConsole::default(); let mut fs = MemoryFileSystem::default(); + let file_path = Path::new("rome.json"); fs.insert( file_path.into(), CONFIG_FORMATTER_IGNORED_DIRECTORIES.as_bytes(), ); - let ignored_file = Path::new("scripts/test.js"); - fs.insert(ignored_file.into(), <&str>::clone(&UNFORMATTED).as_bytes()); - - let file_to_format = Path::new("src/test.js"); - fs.insert(file_to_format.into(), UNFORMATTED.as_bytes()); + const FILES: [(&str, bool); 6] = [ + ("test.js", true), + ("test1.js", false), + ("test2.js", false), + ("test3/test.js", false), + ("test4/test.js", true), + ("test5/test.js", false), + ]; + + for (file_path, _) in FILES { + let file_path = Path::new(file_path); + fs.insert(file_path.into(), UNFORMATTED.as_bytes()); + } let result = run_cli( DynRef::Borrowed(&mut fs), @@ -1128,26 +1137,26 @@ fn does_not_format_ignored_directories() { assert!(result.is_ok(), "run_cli returned {result:?}"); - let mut file = fs - .open(ignored_file) - .expect("formatting target file was removed by the CLI"); + for (file_path, expect_formatted) in FILES { + let mut file = fs + .open(Path::new(file_path)) + .expect("formatting target file was removed by the CLI"); - let mut content = String::new(); - file.read_to_string(&mut content) - .expect("failed to read file from memory FS"); - - assert_eq!(content, UNFORMATTED, "we test the file is not formatted"); - drop(file); - let mut file = fs - .open(file_to_format) - .expect("formatting target file was removed by the CLI"); + let mut content = String::new(); + file.read_to_string(&mut content) + .expect("failed to read file from memory FS"); - let mut content = String::new(); - file.read_to_string(&mut content) - .expect("failed to read file from memory FS"); + let expected = if expect_formatted { + FORMATTED + } else { + UNFORMATTED + }; - assert_eq!(content, FORMATTED, "we test the file is formatted"); - drop(file); + assert_eq!( + content, expected, + "content of {file_path} doesn't match the expected content" + ); + } assert_cli_snapshot(SnapshotPayload::new( module_path!(), diff --git a/crates/rome_cli/tests/configs.rs b/crates/rome_cli/tests/configs.rs index ef9654b34ba..9db71d4663c 100644 --- a/crates/rome_cli/tests/configs.rs +++ b/crates/rome_cli/tests/configs.rs @@ -207,7 +207,13 @@ pub const CONFIG_FORMATTER_AND_FILES_IGNORE: &str = r#"{ pub const CONFIG_FORMATTER_IGNORED_DIRECTORIES: &str = r#"{ "formatter": { - "ignore": ["scripts/*"] + "ignore": [ + "test1.js", + "./test2.js", + "./test3/**/*", + "/test4/**/*", + "test5/**/*" + ] } } "#; diff --git a/crates/rome_cli/tests/snapshots/main_commands_format/does_not_format_ignored_directories.snap b/crates/rome_cli/tests/snapshots/main_commands_format/does_not_format_ignored_directories.snap index 55e4d41eb6e..8f6fb2fd5b6 100644 --- a/crates/rome_cli/tests/snapshots/main_commands_format/does_not_format_ignored_directories.snap +++ b/crates/rome_cli/tests/snapshots/main_commands_format/does_not_format_ignored_directories.snap @@ -7,29 +7,60 @@ expression: content ```json { "formatter": { - "ignore": ["scripts/*"] + "ignore": [ + "test1.js", + "./test2.js", + "./test3/**/*", + "/test4/**/*", + "test5/**/*" + ] } } ``` -## `scripts/test.js` +## `test.js` + +```js +statement(); + +``` + +## `test1.js` + +```js + statement( ) +``` + +## `test2.js` ```js statement( ) ``` -## `src/test.js` +## `test3/test.js` + +```js + statement( ) +``` + +## `test4/test.js` ```js statement(); ``` +## `test5/test.js` + +```js + statement( ) +``` + # Emitted Messages ```block -Formatted 2 file(s) in