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

add ruff pre-commit #1103

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

Conversation

MartinBubel
Copy link
Contributor

Comes at a "problem": ruff identifies almost 800 errors than cannot be fixed automatically and require user interaction.

This is quite tedious and I cannot address it atm as it is just too much work.
Running ruff with --unsafe-fixes brings that number down to just 255 files, but I do not feel comfortable with simply applying those potentially unsafe operations as I am not sure whether the tesitng lib covers all potential issues.

@janmayer any idea / input / experience on this?

@MartinBubel MartinBubel linked an issue Oct 27, 2024 that may be closed by this pull request
@janmayer
Copy link
Contributor

The only thing you can do here is go slow.

First, use ruff format (maybe with some line length config) and ensure that all further commits follow the formatting (via a github action). That will create one massive commit, but as everything is just a format change, it will be save. This will prevent a lot of commit noise in the future (like the whitespace in my earlier PR).

From there on out, go bit by bit - autofix missing imports and import order only, check if everything still works. Its not necessarily truly "save", e.g. if file A imports things from file B which it has imported from C, but are not used in B.

That will probably reduce the number of warnings extensively already.

@janmayer
Copy link
Contributor

janmayer commented Oct 28, 2024

For example, start with pyproject.toml like so

[tool.ruff]
line-length = 120
indent-width = 4
target-version = "py39"

[tool.ruff.lint]
# https://docs.astral.sh/ruff/rules/
select = ["F401", "F404", "E401", "E402", "I001", "I002"]

[tool.ruff.per-file-ignores]
"__init__.py" = ["F401"]  # TODO: Use __all__ in __init__.py

Autofixing will solve quite alot of "problems", but not some sub-cases (F401 IPythonimported but unused; consider usingimportlib.util.find_spec to test for availability to replace the try import blocks, and E402 Module level import not at top of file when it can't tell if its save to move). If you don't want to change these manually yet, you can comment out E402 and F401 afterwards.

@MartinBubel
Copy link
Contributor Author

Thanks for your input on this. I appreciate it!
I'll be back on this sometime later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add ruff linter and formatter
2 participants