Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per file selection #3172

Closed
henryiii opened this issue Feb 23, 2023 · 10 comments · Fixed by #10852
Closed

Per file selection #3172

henryiii opened this issue Feb 23, 2023 · 10 comments · Fixed by #10852
Labels
configuration Related to settings and configuration core Related to core functionality help wanted Contributions especially welcome

Comments

@henryiii
Copy link
Contributor

henryiii commented Feb 23, 2023

I'd like a way to activate a rule for a subset of files; basically the inverse of per-file-ignores. In my case, I'd like to activate pydocstyle on the main module, but not tests, examples, docs, benchmarks, support files, setup.py, etc.

Current and possible options:

  • Use a second ruff.toml/pyproject.toml inside the package. This is not ideal, as I don't want to add extra files (especially pyproject.toml's, as those should only be at the root of packages). This is the only one in the list that would work today (that I know of). .ruff.toml (Additional configuration file .ruff.toml #2988) would help a little, but still not my preferred solution for this case - I'd like all configuration in a single file.
  • Add tool.ruff.per-file-extend-select (technically, per-file-ignores is also extend, so maybe for symmetry it could be tool.ruff.per-file-extend-select). This would then be "src/**.py" = ["D"].
  • Support negation in tool.ruff.per-file-ignores like a gitignore. So "!src/**.py" = ["D"] would ignore D on anything except src/**.py files.

Thoughts?

Here's what I would have to put without this:

[tool.ruff.per-file-ignores]
"doc/**.py" = ["D"]
"tests/**.py" = ["D"]
"bench/**.py" = ["D"]
".ci/**.py" = ["D"]
"cmake_ext.py" = ["D"]
"version.py" = ["D"]
"setup.py" = ["D"]
@charliermarsh
Copy link
Member

I suspect that the last option would be much easier to support than per-file-extend-select. I'm open to it!

@charliermarsh charliermarsh added configuration Related to settings and configuration core Related to core functionality labels Feb 23, 2023
@charliermarsh
Copy link
Member

(The way per-file-ignores work right now is that we still run the ignored rules over those files, we just suppress the violations. We couldn't use the same strategy for per-file-extend-select -- we'd have to enable curating a rule set for every file, which would be a more significant refactor.)

@justinchuby
Copy link
Contributor

justinchuby commented Apr 25, 2023

This feature will be useful for big repositories (eg. PyTorch) to incrementally adopt stricter rules in different components.

Potentially run checks on all rules from the per-file-select as well for everyone, then suppress for files not in this group?

@Kludex
Copy link

Kludex commented Apr 25, 2023

It's useful for small repositories as well. 👀

@zanieb
Copy link
Member

zanieb commented Nov 12, 2023

If someone wants to work on this, I'd be happy to review a proposal. I expect there to be some prototype / design work.

@zanieb zanieb added the help wanted Contributions especially welcome label Nov 12, 2023
@ryangalamb
Copy link

Leaving my notes here in case another contributor wants to pick this up.

The glob mechanism in Ruff is based on globset, and it looks like the options supported there are supported in Ruff.

The only negation option that's supported is single character negation (e.g., [!abc] being the inverse of [abc]). I thought I could be clever and abuse this, but it doesn't seem very reliable. (It also looks awful.)

Extended globbing has negation, which could work, but extended globs are not likely to be supported by globset: BurntSushi/ripgrep#2608

globset is part of the ripgrep project. Ripgrep allows negation by using a ! prefix. Putting a ! in front of your glob will match everything except for the glob. (Link to ripgrep's globbing docs')

The ! prefix approach seems reasonable and easy to explain/document. "!foo/*" would match everything except for "foo/*". Only a ! prefix would be treated specially. Any other !s would be treated as part of the glob.

(And yes, I do realize that this was a lot of work to reach exactly what @henryiii suggested in his third bullet point 😂 Hopefully this helps other curious folks save themselves some time/effort.)

@zanieb Does the ! prefix approach seem reasonable to you?

@charliermarsh
Copy link
Member

I'm supportive of adding negations via !.

@zanieb
Copy link
Member

zanieb commented Apr 5, 2024

I'm supportive as well. Is someone interested in putting up a prototype?

@charliermarsh
Copy link
Member

I think @carljm may be interested.

carljm added a commit that referenced this issue Apr 10, 2024
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.
@carljm
Copy link
Contributor

carljm commented Apr 10, 2024

See some discussion at #10863 (comment) on how negative patterns should behave when the pattern does match the file (i.e. the negative pattern doesn't hit). Should this "un-ignore" the listed patterns, or do nothing (patterns are always only additive to what's ignored)?

carljm added a commit that referenced this issue Apr 12, 2024
Refs #3172 

## Summary

Fix a typo in the docs example, and add a test for the case where a
negative pattern and a positive pattern overlap.

The behavior here is simple: patterns (positive or negative) are always
additive if they hit (i.e. match for a positive pattern, don't match for
a negated pattern). We never "un-ignore" previously-ignored rules based
on a pattern (positive or negative) failing to hit.

It's simple enough that I don't really see other cases we need to add
tests for (the tests we have cover all branches in the ignores_from_path
function that implements the core logic), but open to reviewer feedback.

I also didn't end up changing the docs to explain this more, because I
think they are accurate as written and don't wrongly imply any more
complex behavior. Open to reviewer feedback on this as well!

After some discussion, I think allowing negative patterns to un-ignore
rules is too confusing and easy to get wrong; if we need that, we should
add `per-file-selects` instead.

## Test Plan

Test/docs only change; tests pass, docs render and look right.

---------

Co-authored-by: Alex Waygood <[email protected]>
Glyphack pushed a commit to Glyphack/ruff that referenced this issue Apr 12, 2024
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.
Glyphack pushed a commit to Glyphack/ruff that referenced this issue Apr 12, 2024
…#10863)

Refs astral-sh#3172 

## Summary

Fix a typo in the docs example, and add a test for the case where a
negative pattern and a positive pattern overlap.

The behavior here is simple: patterns (positive or negative) are always
additive if they hit (i.e. match for a positive pattern, don't match for
a negated pattern). We never "un-ignore" previously-ignored rules based
on a pattern (positive or negative) failing to hit.

It's simple enough that I don't really see other cases we need to add
tests for (the tests we have cover all branches in the ignores_from_path
function that implements the core logic), but open to reviewer feedback.

I also didn't end up changing the docs to explain this more, because I
think they are accurate as written and don't wrongly imply any more
complex behavior. Open to reviewer feedback on this as well!

After some discussion, I think allowing negative patterns to un-ignore
rules is too confusing and easy to get wrong; if we need that, we should
add `per-file-selects` instead.

## Test Plan

Test/docs only change; tests pass, docs render and look right.

---------

Co-authored-by: Alex Waygood <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration core Related to core functionality help wanted Contributions especially welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants