fix(rome_lsp): improve the pattern matching logic for ignored files #4024
Conversation
✅ Deploy Preview for docs-rometools canceled.
|
leops
added
A-LSP
|
|
||
| // If a pattern is not absolute, insert a "**" sequence in front | ||
| if !is_absolute { | ||
| tokens.push(AnyRecursiveSequence); |
There was a problem hiding this comment.
What about patterns that already start with a *:
*.js**/*.js
Are these handled correctly (is ***.js a valid pattern?)
There was a problem hiding this comment.
These patterns are handled correctly since the new logic only modifies the internal representation of the pattern and not its string representation, so while I don't think ***.js is a valid pattern this works because it only prepending an "any sequence" in front of the pattern (for the case of *.js this fix also applie to this pattern and makes it work as expected, without it the pattern would match test.js but not src/test.js for instance which is probably not what the user wants)
| 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"))?; |
There was a problem hiding this comment.
What does this macro do?
There was a problem hiding this comment.
The anyhow! macro works like format! except it returns an instance of anyhow::Error instead of a String
| use tower_lsp::lsp_types::VersionedTextDocumentIdentifier; | ||
| use tower_lsp::lsp_types::WorkDoneProgressParams; | ||
| use tower_lsp::LspService; | ||
| use tower_lsp::{jsonrpc::Request, lsp_types::InitializeParams}; | ||
|
|
||
| macro_rules! url { |
There was a problem hiding this comment.
new_url? Also, could we document the macro, please? I feel there's some important information about that "workspace" and windows. I can't understand what this macro is doing...
There was a problem hiding this comment.
I preferred to call the macro url instead of new_url as it is used many times in the tests, and I wanted it to have a short unobtrusive name (like vec! from the standard library, or T! from out syntax crates)
leops
force-pushed
the
fix/ignore-pattern
branch
from
fc6fa63
to
e2c7217
Compare
| @@ -122,6 +122,30 @@ impl Pattern { | |||
| let mut is_recursive = false; | |||
| let mut i = 0; | |||
|
|
|||
| // A pattern is relative if it starts with "." followed by a separator, | |||
| // eg. "./test" or ".\test" | |||
| let is_relative = matches!(chars.get(..2), Some(['.', sep]) if path::is_separator(*sep)); | |||
There was a problem hiding this comment.
TIL: Yu can write if conditions with matches
|
|
||
| #[test] | ||
| fn test_pattern_absolute() { | ||
| assert!(Pattern::new("/a/b") |
There was a problem hiding this comment.
Can we add tests for the *.js and **.js patterns
This picks up a fix for ignoring files: rome/tools#4024
This picks up a fix for ignoring files: rome/tools#4024
This picks up a fix for ignoring files: rome/tools#4024
Summary
Fixes #3790
This PR fixes how ignore patterns are parsed and changes how paths are represented in the language server to reduce discrepancies with the CLI. For ignore patterns, the parse will now strip the
./prefix from the input string, and prepend the pattern with a**group if the input string doesn't start with an absolute filesystem path. For the Language Server, the logic converting URIs intoRomePathwill now try to strip the path to the root of the workspace from the path of the file, which has the effect of making most paths relative much like they generally are on the CLI.Test Plan
I've update the tests for the file ignores on the CLI to check that various forms of ignore patterns are all working correctly. I also modified the LSP tests to ensure it only constructs URIs that can be turned into valid paths for the underlying OS.