From 127aba61c06c31c56a4fbc1de496836df96feb98 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 22 Dec 2022 16:18:48 -0500 Subject: [PATCH 1/2] Respect --force-exclude for files passed via stdin --- src/commands.rs | 17 +++-- src/main.rs | 8 ++- src/resolver.rs | 162 ++++++++++++++++++++++++++++++++++++------------ 3 files changed, 140 insertions(+), 47 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index 25cf801c68c71..66c260f100e21 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -115,17 +115,24 @@ fn read_from_stdin() -> Result { /// Run the linter over a single file, read from `stdin`. pub fn run_stdin( filename: Option<&Path>, - strategy: &PyprojectDiscovery, + pyproject_strategy: &PyprojectDiscovery, + file_strategy: &FileDiscovery, + overrides: &Overrides, autofix: fixer::Mode, ) -> Result { + if let Some(filename) = filename { + if !resolver::python_file_at_path(filename, pyproject_strategy, file_strategy, overrides)? { + return Ok(Diagnostics::default()); + } + } + let settings = match pyproject_strategy { + PyprojectDiscovery::Fixed(settings) => settings, + PyprojectDiscovery::Hierarchical(settings) => settings, + }; let package_root = filename .and_then(std::path::Path::parent) .and_then(packages::detect_package_root); let stdin = read_from_stdin()?; - let settings = match strategy { - PyprojectDiscovery::Fixed(settings) => settings, - PyprojectDiscovery::Hierarchical(settings) => settings, - }; let mut diagnostics = lint_stdin(filename, package_root, &stdin, settings, autofix)?; diagnostics.messages.sort_unstable(); Ok(diagnostics) diff --git a/src/main.rs b/src/main.rs index 75bb08edaf89b..fa933e91dffbd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -224,7 +224,13 @@ fn inner_main() -> Result { // Generate lint violations. let diagnostics = if is_stdin { - commands::run_stdin(cli.stdin_filename.as_deref(), &pyproject_strategy, autofix)? + commands::run_stdin( + cli.stdin_filename.as_deref(), + &pyproject_strategy, + &file_strategy, + &overrides, + autofix, + )? } else { commands::run( &cli.files, diff --git a/src/resolver.rs b/src/resolver.rs index 95ad0f609281d..c8ffd5ac5a669 100644 --- a/src/resolver.rs +++ b/src/resolver.rs @@ -164,8 +164,8 @@ pub fn resolve_settings( /// Return `true` if the given file should be ignored based on the exclusion /// criteria. -fn is_excluded(file_path: &str, file_basename: &str, exclude: &globset::GlobSet) -> bool { - exclude.is_match(file_path) || exclude.is_match(file_basename) +fn match_exclusion(file_path: &str, file_basename: &str, exclusion: &globset::GlobSet) -> bool { + exclusion.is_match(file_path) || exclusion.is_match(file_basename) } /// Return `true` if the `Path` appears to be that of a Python file. @@ -207,35 +207,9 @@ pub fn python_files_in_path( } } - // Omit any paths that are nested within excluded ancestors. This ensures that - // if we pass `subdir/file.py`, and `subdir` is excluded, then we don't - // "miss" that exclusion when walking from `subdir/file.py` below. + // Check if the paths themselves are excluded. if file_strategy.force_exclude { - paths.retain(|path| { - for path in path.ancestors() { - let settings = resolver.resolve(path, pyproject_strategy); - match fs::extract_path_names(path) { - Ok((file_path, file_basename)) => { - if !settings.exclude.is_empty() - && is_excluded(file_path, file_basename, &settings.exclude) - { - debug!("Ignored path via `exclude`: {:?}", path); - return false; - } else if !settings.extend_exclude.is_empty() - && is_excluded(file_path, file_basename, &settings.extend_exclude) - { - debug!("Ignored path via `extend-exclude`: {:?}", path); - return false; - } - } - Err(err) => { - debug!("Ignored path due to error in parsing: {:?}: {}", path, err); - return false; - } - } - } - true - }); + paths.retain(|path| !is_file_excluded(path, &resolver, pyproject_strategy, file_strategy)); if paths.is_empty() { return Ok((vec![], resolver)); } @@ -298,19 +272,23 @@ pub fn python_files_in_path( // Respect our own exclusion behavior. if let Ok(entry) = &result { - if file_strategy.force_exclude || entry.depth() > 0 { + if entry.depth() > 0 { let path = entry.path(); let resolver = resolver.read().unwrap(); let settings = resolver.resolve(path, pyproject_strategy); match fs::extract_path_names(path) { Ok((file_path, file_basename)) => { if !settings.exclude.is_empty() - && is_excluded(file_path, file_basename, &settings.exclude) + && match_exclusion(file_path, file_basename, &settings.exclude) { debug!("Ignored path via `exclude`: {:?}", path); return WalkState::Skip; } else if !settings.extend_exclude.is_empty() - && is_excluded(file_path, file_basename, &settings.extend_exclude) + && match_exclusion( + file_path, + file_basename, + &settings.extend_exclude, + ) { debug!("Ignored path via `extend-exclude`: {:?}", path); return WalkState::Skip; @@ -337,6 +315,108 @@ pub fn python_files_in_path( Ok((files.into_inner().unwrap(), resolver.into_inner().unwrap())) } +/// Return `true` if the Python file at `Path` _not_ excluded. +pub fn python_file_at_path( + path: &Path, + pyproject_strategy: &PyprojectDiscovery, + file_strategy: &FileDiscovery, + overrides: &Overrides, +) -> Result { + if !file_strategy.force_exclude { + return Ok(false); + } + + // Normalize the path (e.g., convert from relative to absolute). + let path = fs::normalize_path(path); + + // Search for `pyproject.toml` files in all parent directories. + let mut resolver = Resolver::default(); + for ancestor in path.ancestors() { + let pyproject = ancestor.join("pyproject.toml"); + if pyproject.is_file() { + if has_ruff_section(&pyproject)? { + let (root, settings) = + resolve_scoped_settings(&pyproject, &Relativity::Parent, Some(overrides))?; + resolver.add(root, settings); + } + } + } + + // Check exclusions. + Ok(!is_file_excluded( + &path, + &resolver, + pyproject_strategy, + file_strategy, + )) +} + +/// Return `true` if the given top-level `Path` should be excluded. +fn is_file_excluded( + path: &Path, + resolver: &Resolver, + pyproject_strategy: &PyprojectDiscovery, + file_strategy: &FileDiscovery, +) -> bool { + // Step 1: Is the file itself, or any of its ancestors, excluded via `exclude` or + // `extend_exclude`? + for path in path.ancestors() { + if path.file_name().is_none() { + break; + } + let settings = resolver.resolve(path, pyproject_strategy); + match fs::extract_path_names(path) { + Ok((file_path, file_basename)) => { + if !settings.exclude.is_empty() + && match_exclusion(file_path, file_basename, &settings.exclude) + { + debug!("Ignored path via `exclude`: {:?}", path); + return true; + } else if !settings.extend_exclude.is_empty() + && match_exclusion(file_path, file_basename, &settings.extend_exclude) + { + debug!("Ignored path via `extend-exclude`: {:?}", path); + return true; + } + } + Err(err) => { + debug!("Ignored path due to error in parsing: {:?}: {}", path, err); + return true; + } + } + } + + // Step 2: Is the file ignored via a gitignore? + // Note that we can't enforce this behavior for non-existent files, which _could_ come up when + // you pass a file via `--stdin-filename`, and _could_ be considered incorrect. For example, + // if `subdir` is listed in the `gitignore`, and you pass `subdir/non_existent_file.py`, then + // right now, we _wouldn't_ mark that theoretical file as ignored. If the `ignore` crate had + // a public matcher API, we could support that, but right now, we _have_ to look at the + // filesystem to reverse-engineer the gitignore match. + if file_strategy.respect_gitignore && path.exists() { + // TODO(charlie): This isn't right either, because the _parent_ could be the thing that's + // ignored via the `.gitignore`. + if let Some(parent) = path.parent() { + for entry in WalkBuilder::new(parent) + .standard_filters(true) + .hidden(false) + .max_depth(Some(1)) + .build() + .flatten() + { + println!("{:?}", entry); + if entry.path() == path { + return false; + } + } + debug!("Ignored path due to gitignore match: {:?}", path); + return true; + } + } + + false +} + #[cfg(test)] mod tests { use std::path::Path; @@ -346,7 +426,7 @@ mod tests { use path_absolutize::Absolutize; use crate::fs; - use crate::resolver::{is_excluded, is_python_path}; + use crate::resolver::{is_python_path, match_exclusion}; use crate::settings::types::FilePattern; #[test] @@ -383,7 +463,7 @@ mod tests { .to_path_buf(), ); let (file_path, file_basename) = fs::extract_path_names(&path)?; - assert!(is_excluded( + assert!(match_exclusion( file_path, file_basename, &make_exclusion(exclude,) @@ -398,7 +478,7 @@ mod tests { .to_path_buf(), ); let (file_path, file_basename) = fs::extract_path_names(&path)?; - assert!(is_excluded( + assert!(match_exclusion( file_path, file_basename, &make_exclusion(exclude,) @@ -415,7 +495,7 @@ mod tests { .to_path_buf(), ); let (file_path, file_basename) = fs::extract_path_names(&path)?; - assert!(is_excluded( + assert!(match_exclusion( file_path, file_basename, &make_exclusion(exclude,) @@ -430,7 +510,7 @@ mod tests { .to_path_buf(), ); let (file_path, file_basename) = fs::extract_path_names(&path)?; - assert!(is_excluded( + assert!(match_exclusion( file_path, file_basename, &make_exclusion(exclude,) @@ -447,7 +527,7 @@ mod tests { .to_path_buf(), ); let (file_path, file_basename) = fs::extract_path_names(&path)?; - assert!(is_excluded( + assert!(match_exclusion( file_path, file_basename, &make_exclusion(exclude,) @@ -464,7 +544,7 @@ mod tests { .to_path_buf(), ); let (file_path, file_basename) = fs::extract_path_names(&path)?; - assert!(is_excluded( + assert!(match_exclusion( file_path, file_basename, &make_exclusion(exclude,) @@ -481,7 +561,7 @@ mod tests { .to_path_buf(), ); let (file_path, file_basename) = fs::extract_path_names(&path)?; - assert!(!is_excluded( + assert!(!match_exclusion( file_path, file_basename, &make_exclusion(exclude,) From f8f09abfc2333d5b816a41ad925c17fc14c37ad7 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 22 Dec 2022 16:31:51 -0500 Subject: [PATCH 2/2] Don't enforce gitignore --- resources/test/project/README.md | 33 +++++++++++++---------- src/commands.rs | 2 +- src/resolver.rs | 46 ++++---------------------------- 3 files changed, 25 insertions(+), 56 deletions(-) diff --git a/resources/test/project/README.md b/resources/test/project/README.md index a521e34cb6a64..978d3d2db6b5e 100644 --- a/resources/test/project/README.md +++ b/resources/test/project/README.md @@ -9,14 +9,14 @@ Running from the repo root should pick up and enforce the appropriate settings f ``` ∴ cargo run resources/test/project/ -Found 7 error(s). resources/test/project/examples/.dotfiles/script.py:1:1: I001 Import block is un-sorted or un-formatted resources/test/project/examples/.dotfiles/script.py:1:8: F401 `numpy` imported but unused resources/test/project/examples/.dotfiles/script.py:2:17: F401 `app.app_file` imported but unused resources/test/project/examples/docs/docs/file.py:1:1: I001 Import block is un-sorted or un-formatted resources/test/project/examples/docs/docs/file.py:8:5: F841 Local variable `x` is assigned to but never used -resources/test/project/src/file.py:1:8: F401 `os` imported but unused -resources/test/project/src/import_file.py:1:1: I001 Import block is un-sorted or un-formatted +resources/test/project/project/file.py:1:8: F401 `os` imported but unused +resources/test/project/project/import_file.py:1:1: I001 Import block is un-sorted or un-formatted +Found 7 error(s). 6 potentially fixable with the --fix option. ``` @@ -24,14 +24,14 @@ Running from the project directory itself should exhibit the same behavior: ``` ∴ (cd resources/test/project/ && cargo run .) -Found 7 error(s). examples/.dotfiles/script.py:1:1: I001 Import block is un-sorted or un-formatted examples/.dotfiles/script.py:1:8: F401 `numpy` imported but unused examples/.dotfiles/script.py:2:17: F401 `app.app_file` imported but unused examples/docs/docs/file.py:1:1: I001 Import block is un-sorted or un-formatted examples/docs/docs/file.py:8:5: F841 Local variable `x` is assigned to but never used -src/file.py:1:8: F401 `os` imported but unused -src/import_file.py:1:1: I001 Import block is un-sorted or un-formatted +project/file.py:1:8: F401 `os` imported but unused +project/import_file.py:1:1: I001 Import block is un-sorted or un-formatted +Found 7 error(s). 6 potentially fixable with the --fix option. ``` @@ -40,9 +40,9 @@ files: ``` ∴ (cd resources/test/project/examples/docs && cargo run .) -Found 2 error(s). docs/file.py:1:1: I001 Import block is un-sorted or un-formatted docs/file.py:8:5: F841 Local variable `x` is assigned to but never used +Found 2 error(s). 1 potentially fixable with the --fix option. ``` @@ -51,8 +51,6 @@ file paths from the current working directory: ``` ∴ (cargo run -- --config=resources/test/project/pyproject.toml resources/test/project/) -Found 11 error(s). -resources/test/project/examples/.dotfiles/script.py:1:1: I001 Import block is un-sorted or un-formatted resources/test/project/examples/.dotfiles/script.py:1:8: F401 `numpy` imported but unused resources/test/project/examples/.dotfiles/script.py:2:17: F401 `app.app_file` imported but unused resources/test/project/examples/docs/docs/concepts/file.py:1:8: F401 `os` imported but unused @@ -61,9 +59,9 @@ resources/test/project/examples/docs/docs/file.py:1:8: F401 `os` imported but un resources/test/project/examples/docs/docs/file.py:3:8: F401 `numpy` imported but unused resources/test/project/examples/docs/docs/file.py:4:27: F401 `docs.concepts.file` imported but unused resources/test/project/examples/excluded/script.py:1:8: F401 `os` imported but unused -resources/test/project/src/file.py:1:8: F401 `os` imported but unused -resources/test/project/src/import_file.py:1:1: I001 Import block is un-sorted or un-formatted -11 potentially fixable with the --fix option. +resources/test/project/project/file.py:1:8: F401 `os` imported but unused +Found 9 error(s). +9 potentially fixable with the --fix option. ``` Running from a parent directory should this "ignore" the `exclude` (hence, `concepts/file.py` gets @@ -71,11 +69,11 @@ included in the output): ``` ∴ (cd resources/test/project/examples && cargo run -- --config=docs/pyproject.toml .) -Found 4 error(s). docs/docs/concepts/file.py:5:5: F841 Local variable `x` is assigned to but never used docs/docs/file.py:1:1: I001 Import block is un-sorted or un-formatted docs/docs/file.py:8:5: F841 Local variable `x` is assigned to but never used excluded/script.py:5:5: F841 Local variable `x` is assigned to but never used +Found 4 error(s). 1 potentially fixable with the --fix option. ``` @@ -83,7 +81,14 @@ Passing an excluded directory directly should report errors in the contained fil ``` ∴ cargo run resources/test/project/examples/excluded/ -Found 1 error(s). resources/test/project/examples/excluded/script.py:1:8: F401 `os` imported but unused +Found 1 error(s). 1 potentially fixable with the --fix option. ``` + +Unless we `--force-exclude`: + +``` +∴ cargo run resources/test/project/examples/excluded/ --force-exclude +∴ cargo run resources/test/project/examples/excluded/script.py --force-exclude +``` diff --git a/src/commands.rs b/src/commands.rs index 66c260f100e21..d06e2ecb6f9b6 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -130,7 +130,7 @@ pub fn run_stdin( PyprojectDiscovery::Hierarchical(settings) => settings, }; let package_root = filename - .and_then(std::path::Path::parent) + .and_then(Path::parent) .and_then(packages::detect_package_root); let stdin = read_from_stdin()?; let mut diagnostics = lint_stdin(filename, package_root, &stdin, settings, autofix)?; diff --git a/src/resolver.rs b/src/resolver.rs index c8ffd5ac5a669..bf103c6b6e921 100644 --- a/src/resolver.rs +++ b/src/resolver.rs @@ -209,7 +209,7 @@ pub fn python_files_in_path( // Check if the paths themselves are excluded. if file_strategy.force_exclude { - paths.retain(|path| !is_file_excluded(path, &resolver, pyproject_strategy, file_strategy)); + paths.retain(|path| !is_file_excluded(path, &resolver, pyproject_strategy)); if paths.is_empty() { return Ok((vec![], resolver)); } @@ -315,7 +315,7 @@ pub fn python_files_in_path( Ok((files.into_inner().unwrap(), resolver.into_inner().unwrap())) } -/// Return `true` if the Python file at `Path` _not_ excluded. +/// Return `true` if the Python file at `Path` is _not_ excluded. pub fn python_file_at_path( path: &Path, pyproject_strategy: &PyprojectDiscovery, @@ -323,7 +323,7 @@ pub fn python_file_at_path( overrides: &Overrides, ) -> Result { if !file_strategy.force_exclude { - return Ok(false); + return Ok(true); } // Normalize the path (e.g., convert from relative to absolute). @@ -343,12 +343,7 @@ pub fn python_file_at_path( } // Check exclusions. - Ok(!is_file_excluded( - &path, - &resolver, - pyproject_strategy, - file_strategy, - )) + Ok(!is_file_excluded(&path, &resolver, pyproject_strategy)) } /// Return `true` if the given top-level `Path` should be excluded. @@ -356,10 +351,8 @@ fn is_file_excluded( path: &Path, resolver: &Resolver, pyproject_strategy: &PyprojectDiscovery, - file_strategy: &FileDiscovery, ) -> bool { - // Step 1: Is the file itself, or any of its ancestors, excluded via `exclude` or - // `extend_exclude`? + // TODO(charlie): Respect gitignore. for path in path.ancestors() { if path.file_name().is_none() { break; @@ -385,35 +378,6 @@ fn is_file_excluded( } } } - - // Step 2: Is the file ignored via a gitignore? - // Note that we can't enforce this behavior for non-existent files, which _could_ come up when - // you pass a file via `--stdin-filename`, and _could_ be considered incorrect. For example, - // if `subdir` is listed in the `gitignore`, and you pass `subdir/non_existent_file.py`, then - // right now, we _wouldn't_ mark that theoretical file as ignored. If the `ignore` crate had - // a public matcher API, we could support that, but right now, we _have_ to look at the - // filesystem to reverse-engineer the gitignore match. - if file_strategy.respect_gitignore && path.exists() { - // TODO(charlie): This isn't right either, because the _parent_ could be the thing that's - // ignored via the `.gitignore`. - if let Some(parent) = path.parent() { - for entry in WalkBuilder::new(parent) - .standard_filters(true) - .hidden(false) - .max_depth(Some(1)) - .build() - .flatten() - { - println!("{:?}", entry); - if entry.path() == path { - return false; - } - } - debug!("Ignored path due to gitignore match: {:?}", path); - return true; - } - } - false }