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

Use ruff for linting and code formatting #1019

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jkarasti
Copy link
Contributor

@jkarasti jkarasti commented Dec 2, 2024

Replace black and isort with ruff for linting and code formatting and fixes everything found by ruffs default configuration. Also add one fix to rule them all from freedomofpress/securedrop-tooling/issues/11 and a complementary check target that runs ruff check, ruff format and mypy useful for CI. Note that this "reverts" #904.

This is ready for merge as is but not complete as I'd I'd like to enable more lint rules.

Closes #254

Copy link
Contributor

@almet almet left a comment

Choose a reason for hiding this comment

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

Thanks @jkarasti for the work on this :-)

Here is some feedback:

  • To me, make check conveys less meaning than make lint, so we might maybe keep the last one.
  • We are currently using isort in addition to ruff, but ruff can do this for us. I think we should do this before merging.

Thanks a lot 👍 let me know when you need another review :-)

Makefile Outdated Show resolved Hide resolved
@jkarasti
Copy link
Contributor Author

jkarasti commented Dec 2, 2024

Here is some feedback:

* To me, `make check` conveys less meaning than `make lint`, so we might maybe keep the last one.

My thinking was to have a complementary target for fix so that there would be one target used to check for linting, formatting and typing issues and one target used to fix linting and formatting issues (mypy doesen't have a way to automatically apply fixes it seems). I can switch it back if you would prefer lint instead. These could also be lint/lint-check and lint-fix depending on if we want to follow what Securedrop devs are doing or not.

* We are currently using `isort` in addition to ruff, but ruff can [do this for us](https://docs.astral.sh/ruff/rules/#output-format). I think we should do this before merging.

I've enabled the isort rules.

Copy link
Contributor

@almet almet left a comment

Choose a reason for hiding this comment

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

Thanks for enabling the I rules for isort, and for explaining your thinking.

Rather than having detailed targets, regrouping them behind the fix and lint targets seems to cut it while being self-explanatory. What do you think?

It's what the securedrop folks are doing, and having the same naming seems the right thing to do. Here's what's inside their makefile:

.PHONY: lint
lint:
	@ruff check .
	@ruff format --check .

.PHONY: fix
fix:
	@ruff check . --fix
	@ruff format .

Also, thanks for enabling the isort rule. The linter is currently failing with:

dangerzone/args.py:109: error: Unused "type: ignore" comment  [unused-ignore]

@almet almet added this to the 0.9.0 milestone Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: PR Review
Development

Successfully merging this pull request may close these issues.

Use ruff for linting, code formatting and module sorting
2 participants