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

tools/ruff.sh: Allow restricting checking to specific subtrees #3395

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

PeterNerlich
Copy link
Collaborator

Short description

With #3333 just merged, ruff became our main linting tool. However if you're like me and have a lot of messy files and experiments lying around your working directory, you might want to restrict ruff to only look at a few subtrees in your project. While the performance improvement barely matters because ruff is already so fast, you will appreciate having irrelevant files untouched and ruff only printing messages about files you care about.

Proposed changes

  • Parse arguments to tools/ruff.sh and save them to PATHS
  • Set PATHS to BASE_DIR if none are supplied
  • Run both ruff check --fix and ruff format only for PATHS

Side effects

  • None, this change is fully backwards compatible to the previous intended usage. But if you relied on the tool ignoring any arguments at the end, it will now either complain that the file doesn't exist or lint those files.

Pull Request Review Guidelines

@PeterNerlich PeterNerlich added infrastructure Issues related to the dev or production environment enhancement This improves an existing feature labels Feb 4, 2025
@charludo charludo self-assigned this Feb 5, 2025
Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I think this is a great idea! I have an extension of this in mind though, which also brings some simplification. It looks like ruff check and ruff format --check do the exact same thing as ruff check . and ruff format --check ., meaning we could just default to not providing a path at all, and simply pass all arguments given to ./tools/ruff.sh to both of those calls, the same way we're already doing for the pre-commit hooks.

Cons:

  • no checking if passed paths actually exist. Do you have a specific usecase for this in mind? Otherwise I think ruff's own error message is already pretty succinct:
    error: Failed to format help-i-dont-exist: No such file or directory (os error 2)
    

Pros:

  • much simpler logic. Just need to change
    ruff check --fix . || true
    ruff format .
    to
    ruff check --fix "$@" || true
    ruff format "$@"
  • more importantly though, that makes it trivial to pass additional flags to ruff. For example, I have really been wanting a way to append --unsafe-fixes a lot lately 😁
    (OK, to be fair this will throw some errors because format and check don't share a lot of flags, but it would certainly still be servicable)

@PeterNerlich
Copy link
Collaborator Author

we could just default to not providing a path at all, and simply pass all arguments given to ./tools/ruff.sh to both of those calls

Yeah I'm also fine with that, I just copied it from tools/test.sh and removed everything that was tests-specific… as in, everything but the overarching structure :D

(OK, to be fair this will throw some errors because format and check don't share a lot of flags, but it would certainly still be servicable)

Yes I tried to look it up but didn't immediately find a comprehensive list of available options for both commands, so I just left it at the paths. I was wanted to keep the structure for this actually, so we could add flags and what to do with it later (to add it to one or the other ruff command, or some custom logic)

@charludo
Copy link
Contributor

charludo commented Feb 6, 2025

we could just default to not providing a path at all, and simply pass all arguments given to ./tools/ruff.sh to both of those calls

Yeah I'm also fine with that, I just copied it from tools/test.sh and removed everything that was tests-specific… as in, everything but the overarching structure :D

(OK, to be fair this will throw some errors because format and check don't share a lot of flags, but it would certainly still be servicable)

Yes I tried to look it up but didn't immediately find a comprehensive list of available options for both commands, so I just left it at the paths. I was wanted to keep the structure for this actually, so we could add flags and what to do with it later (to add it to one or the other ruff command, or some custom logic)

Hm. It would probably be easier to split the tool into two (ruff_format.sh, ruff_check.sh) and call both from ruff.sh than to mainting a list of what-arg-goes-where for every conceivable usecase.

The custom logic right now is not doing anything except remove non-existing paths, and print an error similar to what ruff would do anyways, right? IMO that is not enough benefit to justify 25 lines of shellscript 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This improves an existing feature infrastructure Issues related to the dev or production environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants