trezor / trezor-firmware

:lock: Trezor Firmware Monorepo
https://trezor.io
Other
1.38k stars 665 forks source link

Consider using ruff as the one and only linter of python code #2820

Open grdddj opened 1 year ago

grdddj commented 1 year ago

If all what ruff promises to do now is true, we should be able to replace all non-type checking static analysis (isort, black, flake8 and pylint) just with this one tool, which should also be lightning-quick, with autofixing and much more nice features.

matejcik commented 1 year ago

black

a formatter not a linter, ruff doesn't seem to claim formatting functionality

isort

not sure if there's import sorting built in?

pylint

this is the big one. we essentially use a whitelist of pylint rules. if ruff can do all or most of them, we would get a neat win just from replacing pylint.

matejcik commented 1 year ago

we also have some flake8 plugin or other, i think for checking package requirements?

grdddj commented 1 year ago

black

a formatter not a linter, ruff doesn't seem to claim formatting functionality

Right - https://github.com/charliermarsh/ruff#is-ruff-compatible-with-black. Luckily black is quite quick on our codebase.

isort

not sure if there's import sorting built in?

Not completely sure, https://github.com/charliermarsh/ruff#isort-i says it can do unsorted-imports, so it might even have a way to autofix them

we also have some flake8 plugin or other, i think for checking package requirements?

We have flake8-requirements. That I did not find in the list of supported rules - https://github.com/charliermarsh/ruff#table-of-contents


One big problem I have is the inability to even run ruff on my NixOS machine - I can pip install ruff anywhere, it will go fine, is available in python shell as import ruff, but always getting ruff: command not found when trying to use it as a CLI.

On Windows, it works fine, so I guess NixOS will be the issue.

prusnak commented 1 year ago

I guess NixOS will be the issue.

ruff is packaged in nixpkgs, so nixos is not an issue

grdddj commented 1 year ago

I guess NixOS will be the issue.

ruff is packaged in nixpkgs, so nixos is not an issue

Thanks for this info! It was possible to add it to our shell.nix and use it. Added as a dependency in https://github.com/trezor/trezor-firmware/pull/3207 - still with some issues, at least on my machine, I cannot use it from Makefile

matejcik commented 9 months ago

Right now it appears that ruff can fully replace black, isort and flake8. Unclear about pylint, but I suspect that having ruff's rules is better than having pylint rules.

Two blockers for full replacement are flake8-requirements (as mentioned above) -- but (a) that's only relevant for trezorlib which has its own make style target and can easily run both ruff and the original flake8 (reasonably fast).

And the custom pylint plugin that detects async def -> Awaitable[T], which of course depends on pylint, which is a major slowdown. The logic is easy enough to rewrite in pretty much any other AST parser though. While ruff doesn't have plugins yet, it might be easiest to grab whatever Python AST library ruff is using and just writing the async-awaitable detection in that.

matejcik commented 9 months ago

pylint rules available: https://docs.astral.sh/ruff/rules/#pylint-pl

prusnak commented 9 months ago

Right now it appears that ruff can fully replace black, isort and flake8.

Afaik not black, but it works in tandem with black very well.