Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions crates/ruff/tests/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,35 @@ OTHER = "OTHER"
Ok(())
}

/// Regression test for <https://github.com/astral-sh/ruff/issues/20035>
#[test]
fn deduplicate_directory_and_explicit_file() -> Result<()> {
let tempdir = TempDir::new()?;
let root = tempdir.path();

let main = root.join("main.py");
fs::write(&main, "x = 1\n")?;

assert_cmd_snapshot!(
Command::new(get_cargo_bin(BIN_NAME))
.current_dir(root)
.args(["format", "--no-cache", "--check"])
.arg(".")
.arg("main.py"),
@r"
success: false
exit_code: 1
----- stdout -----
Would reformat: main.py
1 file would be reformatted

----- stderr -----
"
);

Ok(())
}

#[test]
fn syntax_error() -> Result<()> {
let tempdir = TempDir::new()?;
Expand Down
34 changes: 34 additions & 0 deletions crates/ruff/tests/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,40 @@ OTHER = "OTHER"
Ok(())
}

/// Regression test for <https://github.com/astral-sh/ruff/issues/20035>
#[test]
fn deduplicate_directory_and_explicit_file() -> Result<()> {
Comment thread
ntBre marked this conversation as resolved.
let tempdir = TempDir::new()?;
let root = tempdir.path();

let main = root.join("main.py");
fs::write(&main, "import os\n")?;

insta::with_settings!({
filters => vec![(tempdir_filter(&tempdir).as_str(), "[TMP]/")]
}, {
assert_cmd_snapshot!(
Command::new(get_cargo_bin(BIN_NAME))
.current_dir(root)
.args(STDIN_BASE_OPTIONS)
.arg(".")
.arg("main.py"),
@r"
success: false
exit_code: 1
----- stdout -----
main.py:1:8: F401 [*] `os` imported but unused
Found 1 error.
[*] 1 fixable with the `--fix` option.

----- stderr -----
"
);
});

Ok(())
}

#[test]
fn exclude_stdin() -> Result<()> {
let tempdir = TempDir::new()?;
Expand Down
74 changes: 60 additions & 14 deletions crates/ruff_workspace/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,10 +523,42 @@ impl<'config> WalkPythonFilesState<'config> {
let (files, error) = self.merged.into_inner().unwrap();
error?;

Ok((files, self.resolver.into_inner().unwrap()))
let deduplicated_files = deduplicate_files(files);

Ok((deduplicated_files, self.resolver.into_inner().unwrap()))
}
}

/// Deduplicate files by path, prioritizing `Root` files over `Nested` files.
///
/// When the same path appears both as a directly specified input (`Root`)
/// and via directory traversal (`Nested`), keep the `Root` entry and drop
/// the `Nested` entry.
///
/// Dropping the root entry means that the explicitly passed path may be
/// unintentionally ignored, since it is treated as nested and can be excluded
/// despite being requested.
///
/// Concretely, with `lint.exclude = ["foo.py"]` and `ruff check . foo.py`,
/// we must keep `Root(foo.py)` and drop `Nested(foo.py)` so `foo.py` is
/// linted as the user requested.
fn deduplicate_files(mut files: ResolvedFiles) -> ResolvedFiles {
// Sort by path; for identical paths, prefer Root over Nested; place errors after files
files.sort_by(|a, b| match (a, b) {
(Ok(a_file), Ok(b_file)) => a_file.cmp(b_file),
(Ok(_), Err(_)) => Ordering::Less,
(Err(_), Ok(_)) => Ordering::Greater,
(Err(_), Err(_)) => Ordering::Equal,
});
Comment thread
TaKO8Ki marked this conversation as resolved.

files.dedup_by(|a, b| match (a, b) {
(Ok(a_file), Ok(b_file)) => a_file.path() == b_file.path(),
_ => false,
});

files
}

struct PythonFilesVisitorBuilder<'s, 'config> {
state: &'s WalkPythonFilesState<'config>,
transformer: &'s (dyn ConfigurationTransformer + Sync),
Expand Down Expand Up @@ -682,7 +714,7 @@ impl Drop for PythonFilesVisitor<'_, '_> {
}
}

#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq, Ord, PartialOrd)]
pub enum ResolvedFile {
/// File explicitly passed to the CLI
Root(PathBuf),
Expand Down Expand Up @@ -715,18 +747,6 @@ impl ResolvedFile {
}
}

impl PartialOrd for ResolvedFile {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}

impl Ord for ResolvedFile {
fn cmp(&self, other: &Self) -> Ordering {
self.path().cmp(other.path())
}
}

/// Return `true` if the Python file at [`Path`] is _not_ excluded.
pub fn python_file_at_path(
path: &Path,
Expand Down Expand Up @@ -1003,6 +1023,32 @@ mod tests {
Ok(())
}

#[test]
fn find_python_files_deduplicated() -> Result<()> {
Comment thread
TaKO8Ki marked this conversation as resolved.
Outdated
Comment thread
ntBre marked this conversation as resolved.
Outdated
// Initialize the filesystem:
// root
// ├── file1.py
let tmp_dir = TempDir::new()?;
let root = tmp_dir.path();
let file1 = root.join("file1.py");
File::create(&file1)?;

let (paths, _) = python_files_in_path(
&[root.to_path_buf(), file1.clone()],
&PyprojectConfig::new(PyprojectDiscoveryStrategy::Fixed, Settings::default(), None),
&NoOpTransformer,
)?;
let paths = paths
.into_iter()
.flatten()
.map(ResolvedFile::into_path)
.sorted()
.collect::<Vec<_>>();
assert_eq!(paths, [file1]);

Ok(())
}

fn make_exclusion(file_pattern: FilePattern) -> GlobSet {
let mut builder = globset::GlobSetBuilder::new();
file_pattern.add_to(&mut builder).unwrap();
Expand Down
Loading