search

Open-MSS / MSS

A QT application, a OGC web map server, a collaboration server to plan atmospheric research flights.
https://open-mss.github.io
Apache License 2.0
56 stars 68 forks source link

Consider replacing flake8 with ruff #2145

Open matrss opened 7 months ago

matrss commented 7 months ago

Ruff is "An extremely fast Python linter and code formatter, written in Rust." (https://github.com/astral-sh/ruff)

It effectively does what flake8 does, with some of its plugins (flake8-builtins, flake8-bugbear, and many more) added and it claims to be (and probably is) faster. Optionally it can also do import sorting like isort and code formatting like black (but we don't have to use those).

This came up in #2144.

ReimarBauer commented 7 months ago

@joernu76 do you remember the issue we had with black?

joernu76 commented 7 months ago

black was rather aggressive and, let's say controversial, with its formatting.

the setup.py/flake8 integration isn't working anymore, but flake8 itself is a nice checker.

Reformatting source code cannot be done in the pipeline, but would have to be done on the users systems before commit via hooks.

We can certainly add ruff as a checker in addition to the current flow and compare the results.

matrss commented 7 months ago

I've found this older discussions related to black, which I think Reimar initially referenced in #2144 regarding black breaking MSS: https://github.com/Open-MSS/MSS/pull/618#discussion_r553611510.

black was rather aggressive and, let's say controversial, with its formatting.

I've found the black formatting style to be very pleasant to read, compared to more inconsistent formatting like we have here. Yes, it is very aggressive and very much opinionated, but I think that is its greatest strength (i.e. no discussion about how to format a line of code, apply black and be done with it).

I was also able to format the entirety of MSS with ruff's formatter and also with its import sorter and have the test suite pass without issue. So maybe the concerns that this breaks code no longer apply.

Reformatting source code cannot be done in the pipeline, but would have to be done on the users systems before commit via hooks.

Not necessarily via hooks, but yes, CI should just check that the formatting is OK and not apply changes itself (just like what the flake8 action does).

In general I like to apply a linter like flake8 or ruff mostly in their standard configuration, because that removes a lot of potential for bikeshedding about what code should look like. That means I would prefer the CI action to apply linting to the entirety of this repository (not just mslib/ and tests/ like right now; but the excludes for autogenerated stuff are reasonable) and I'd like to not have the ignore = E124,E125,E402,W504 we have right now for flake8, because I think all of those linting rules are valid and the code would look better if they were applied (of course there might be some locations where a linting rule does not apply, e.g. E402 with some of the imports in conftest.py because they cannot be at the top, but those should be ignored for those specific locations and not everywhere, in my opinion).

But I also don't want to force my aesthetic preference on this project if someone has a differing opinion.

ReimarBauer commented 3 months ago

@joernu76 please have a look here

matrss commented 3 months ago

ruff also has another major advantage over flake8: it respects gitignore, so it automatically skips all files that are also ignored by git.