Support negated patterns in [extend-]per-file-ignores #10852
Conversation
| if negated { | ||
| pattern.drain(..1); | ||
| } |
There was a problem hiding this comment.
This might look a little odd, but it's zero-allocation, unlike the version using .strip_prefix().
Perf probably doesn't matter much here in config-parsing; if you'd prefer the version that takes a non-mut String and uses .strip_prefix(), I can switch to that, though the code actually ends up longer than this.
There was a problem hiding this comment.
\cc @BurntSushi may have interesting things to say here :)
There was a problem hiding this comment.
I think this is fine personally. I agree that saving an alloc probably doesn't matter here (I assume these ultimately get converted to a glob matcher, and that is an enormous amount of work compared to saving an alloc) so I'd write whatever is clear. And this seems clear to me. :)
| @@ -593,7 +603,7 @@ pub type IdentifierPattern = glob::Pattern; | |||
| #[derive(Debug, Clone, CacheKey, Default)] | |||
| pub struct PerFileIgnores { | |||
| // Ordered as (absolute path matcher, basename matcher, rules) | |||
| ignores: Vec<(GlobMatcher, GlobMatcher, RuleSet)>, | |||
| ignores: Vec<(GlobMatcher, GlobMatcher, bool, RuleSet)>, | |||
There was a problem hiding this comment.
I think this should be a named struct (CompiledPerFileIgnore? PerFileIgnoreMatcher?) with named fields rather than a tuple, but for ease of review I didn't make that change here; will push it as a separate PR (unless a reviewer suggests I shouldn't.)
|
| @@ -914,6 +915,8 @@ pub struct LintCommonOptions { | |||
| # Ignore `E402` (import violations) in all `__init__.py` files, and in `path/to/file.py`. | |||
| "__init__.py" = ["E402"] | |||
| "path/to/file.py" = ["E402"] | |||
| # Ignore `D` rules everywhere except for the `src/` directory. | |||
There was a problem hiding this comment.
Is this comment correct? Looks like it ignored F401 not D
There was a problem hiding this comment.
Ah yeah, it should be D below (or the comment should change to F401) \cc @carljm
There was a problem hiding this comment.
Oops, thanks, will fix!
| "###); | ||
| }); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
It might be worth adding some more tests where there are both regular patterns and negated patterns. And in particular, cases where a negated pattern might try to "override" a previous pattern. e.g.,
"*.py" = ["RUF"]
"!foo.py" = ["RUF"]
Or something like that.
There was a problem hiding this comment.
Yeah, that's a great point, I'll put up an update with a test for this case, and probably also more clearly document how this works.
(I don't expect to change the implementation as part of this; I think the way it needs to work is how it does now: we go through each pattern, and if the pattern matches (whether that's a positive match or a negated non-match), we add the listed rules as ignored; we never un-ignore rules based on failure to match a pattern. I think that would get too complex to understand pretty quickly, and also un-ignoring doesn't really make sense for an option named per-file-ignores.)
| if negated { | ||
| pattern.drain(..1); | ||
| } |
There was a problem hiding this comment.
I think this is fine personally. I agree that saving an alloc probably doesn't matter here (I assume these ultimately get converted to a glob matcher, and that is an enormous amount of work compared to saving an alloc) so I'd write whatever is clear. And this seems clear to me. :)
Fixes astral-sh#3172 ## Summary Allow prefixing [extend-]per-file-ignores patterns with `!` to negate the pattern; listed rules / prefixes will be ignored in all files that don't match the pattern. ## Test Plan Added tests for the feature. Rendered docs and checked rendered output.
Fixes #3172
Summary
Allow prefixing [extend-]per-file-ignores patterns with
!to negate the pattern; listed rules / prefixes will be ignored in all files that don't match the pattern.Test Plan
Added tests for the feature.
Rendered docs and checked rendered output.