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

Have ruff apply to securedrop/scripts/ too #6932

Merged
merged 3 commits into from Sep 27, 2023
Merged

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Aug 24, 2023

Status

Ready for review

Description of Changes

These need explicit mentioning since they don't have a .py extension.

The E402 error now applies to the imports following the sys.path.append() call, so move them around.

KOG Update:

Testing

  • Visual review
  • CI passes

Checklist

  • Linting (make lint) and tests (make test) pass in the development container

@legoktm legoktm requested a review from a team as a code owner August 24, 2023 10:22
@cfm cfm self-requested a review August 24, 2023 20:46
@cfm cfm self-assigned this Aug 24, 2023
@legoktm
Copy link
Member Author

legoktm commented Aug 25, 2023

updated the precommit hook to stay limited to .py files, as ruff got indigestion trying to lint a bash script.

This shouldn't be necessary, running ruff check . should only lint Python files already, since that's what make ruff does...

@legoktm
Copy link
Member Author

legoktm commented Aug 25, 2023

I edited the commit message to fix the issue. I kept the precommit hook commit, but I think it should be dropped or needs more detail on what went wrong.

@zenmonkeykstop
Copy link
Contributor

Oh - iirc it was trying to lint the run-mypy bash script (which was also a problem due to a bad regex in the original hook).

@rocodes
Copy link
Contributor

rocodes commented Sep 20, 2023

I'm also mildly confused-- reverting to 3f4353e (so no changes to the pre-commit hook) also works just fine for me, and make typelint, make lint, make rust-lint etc are all perfectly happy without that change.

@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Sep 25, 2023

I'm also mildly confused-- reverting to 3f4353e (so no changes to the pre-commit hook) also works just fine for me, and make typelint, make lint, make rust-lint etc are all perfectly happy without that change.

The error is only triggered if there's a change in run-mypy. Try:

  • reverting the commit with the hook change
  • adding a trivial change (# i'm a comment!) in securedrop/bin/run-mypy
  • adding and committing it

It's happening because the hook only checks anything if there is a match for *py files, and ruff is trying to parse run-mypy as a .py file. Changing the regex to match *.py and running ruff on only matching files solves that. (Yes it's super edge-casey.)

@legoktm
Copy link
Member Author

legoktm commented Sep 25, 2023

Thanks for the details:

and ruff is trying to parse run-mypy as a .py file

This is the real problem IMO because it means that even make ruff could trigger it. So I'll look into fixing that filtering and having the git hook continue to call plain ruff check .

@legoktm
Copy link
Member Author

legoktm commented Sep 25, 2023

@zenmonkeykstop what's the output of ruff check . --show-files on your machine? On mine it doesn't include run-mypy so ruff really shouldn't be linting it...

@rocodes
Copy link
Contributor

rocodes commented Sep 26, 2023

(I'm not kev, but I am chiming in to say that run-mypy is also not in my list of files checked by ruff per ruff check . --show-files)

@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Sep 26, 2023

I'm so dumb - the failure comes from the black check immediately beforehand in the hook, which does take a list of files. Sooo the regex still needs fixing for that case, but the resultant list of files is fed to black, not ruff.

@rocodes
Copy link
Contributor

rocodes commented Sep 27, 2023

(Ready for re-review)

@@ -14,7 +14,7 @@ if [[ -n "$PY_FILES" ]]; then
echo "$PY_FILES"
# Run black against changed python files for this commit
black --check --diff ${PY_FILES//$'\n'/ }
# Run ruff (against all files, it's fast enough)
# Run ruff against changed python files
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment should be left as it was originally

legoktm and others added 3 commits September 27, 2023 10:32
These need explicit mentioning since they don't have a `.py` extension.

The E402 error now applies to the imports following the sys.path.append()
call, so move them around.
The relative path causes an issue on some systems.

Fixes #6930.
Copy link
Member Author

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

LGTM! (can't approve my own PR)

Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

also LGTM

@zenmonkeykstop zenmonkeykstop merged commit 1016f48 into develop Sep 27, 2023
9 checks passed
@legoktm legoktm deleted the ruff-scripts branch September 27, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants