Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Commit

Permalink
fix(rome_lsp): improve the pattern matching logic for ignored files (#…
Browse files Browse the repository at this point in the history
…4024)

* fix(rome_service): improve the pattern matching logic

* strip the workspace root on paths in the language server

* update tests

* add additional test cases

* add tests for glob patterns
  • Loading branch information
leops authored Dec 13, 2022
1 parent a090d60 commit e6c6a4b
Show file tree
Hide file tree
Showing 11 changed files with 250 additions and 60 deletions.
56 changes: 34 additions & 22 deletions crates/rome_cli/tests/commands/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1104,17 +1104,29 @@ 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); 9] = [
("test.js", true),
("test1.js", false),
("test2.js", false),
("test3/test.js", false),
("test4/test.js", true),
("test5/test.js", false),
("test6/test.js", false),
("test/test.test7.js", false),
("test.test7.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),
Expand All @@ -1128,26 +1140,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!(),
Expand Down
10 changes: 9 additions & 1 deletion crates/rome_cli/tests/configs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,15 @@ 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/**/*",
"**/test6/*.js",
"*.test7.js"
]
}
}
"#;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,80 @@ expression: content
```json
{
"formatter": {
"ignore": ["scripts/*"]
"ignore": [
"test1.js",
"./test2.js",
"./test3/**/*",
"/test4/**/*",
"test5/**/*",
"**/test6/*.js",
"*.test7.js"
]
}
}

```

## `scripts/test.js`
## `test.js`

```js
statement();

```

## `test.test7.js`

```js
statement( )
```

## `test/test.test7.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( )
```

## `test6/test.js`

```js
statement( )
```

# Emitted Messages

```block
Formatted 2 file(s) in <TIME>
Formatted 3 file(s) in <TIME>
```


4 changes: 2 additions & 2 deletions crates/rome_lsp/src/handlers/analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub(crate) fn code_actions(
params: CodeActionParams,
) -> Result<Option<CodeActionResponse>> {
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,
Expand All @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions crates/rome_lsp/src/handlers/formatting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub(crate) fn format(
params: DocumentFormattingParams,
) -> Result<Option<Vec<TextEdit>>> {
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)?;

Expand Down Expand Up @@ -56,7 +56,7 @@ pub(crate) fn format_range(
params: DocumentRangeFormattingParams,
) -> Result<Option<Vec<TextEdit>>> {
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(|| {
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion crates/rome_lsp/src/handlers/rename.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use tracing::trace;
#[tracing::instrument(level = "debug", skip(session), err)]
pub(crate) fn rename(session: &Session, params: RenameParams) -> Result<Option<WorkspaceEdit>> {
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...");

Expand Down
6 changes: 3 additions & 3 deletions crates/rome_lsp/src/handlers/text_document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub(crate) async fn did_open(
let content = params.text_document.text;
let language_hint = Language::from_language_id(&params.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 {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/rome_lsp/src/requests/syntax_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub struct SyntaxTreePayload {

pub(crate) fn syntax_tree(session: &Session, url: &Url) -> Result<String> {
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 })?;
Expand Down
25 changes: 21 additions & 4 deletions crates/rome_lsp/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -141,17 +142,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<RomePath> {
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,
Expand Down
Loading

0 comments on commit e6c6a4b

Please sign in to comment.