update pre-commit hooks

COVESA / vss-tools

Software for working with VSS (https://github.com/COVESA/vehicle_signal_specification)

Mozilla Public License 2.0

49 stars 51 forks source link

update pre-commit hooks #363

Open nwesem opened 1 month ago

nwesem commented 1 month ago

Hi everyone,

as mentioned in issue #341, in my opinion it makes sense to use more pre-commit hooks to enforce proper formatting, correct spelling, and so on.. in all files that will be changed from now on. It will be a bit more difficult to pass the pipeline but it will also increase code quality.

This is still open for discussion though. I am happy to receive any comments

erikbosch commented 1 month ago

I am in general positive. Some things came to my mind.

Should we possibly document why we introduce the respective hooks. In the commit, in the PR or possibly in the file? Like, why do we add ruff when we already have flake8. Not saying that it is bad, maybe there are benefits having both, but possibly also a risk for conflicts if they have different rules.
Have you done some tests on how much this will change files, i.e. how much errors/changes will you get if trying to do a random edit in one of our larger Python files? I.e. does it seem feasible to update existing code whenever someone touches a file, or should we better handle the "bigger" files as a separate update activity to make sure they comply with the rules.

erikbosch commented 1 month ago

MoM: Please Review

nwesem commented 1 month ago

Sry for the late reply.. According to this blog post it is best to use ruff in pre-commit only for python >=3.11, so I removed it for now. But this is still open for discussion, does it make sense to add more modules to our pre-commit config?

erikbosch commented 4 weeks ago

Sry for the late reply.. According to this blog post it is best to use ruff in pre-commit only for python >=3.11, so I removed it for now. But this is still open for discussion, does it make sense to add more modules to our pre-commit config?

I read the blog post somewhat differently, that it now supports 3.11 (but also older version)

Until February 2023, Ruff lacked support for pattern matching, a feature introduced in Python 3.10, which was a showstopper for many considering Ruff adoption. Luckily, that got fixed (relevant issue) and currently Ruff claims to be compatible with 3.11

The homepage states support for 3.7-3.13

nwesem commented 4 weeks ago

hi @erikbosch, you are right. I was referring to this paragraph and now I also read it differently. I would suggest we switch to ruff as it is much faster (written in rust) and find a configuration that matches the current one. For flake8 we only use the max_line_length = 120 which we could easily apply in the pyproject.toml (which is one way to configure ruff)

erikbosch commented 4 weeks ago

Feel free to revert to ruff. Concern config, I assume this change may have some minor effects on #367 as it mentions flake8 as dev dependencies. I did by the way do some tests on both flake8 and ruff at https://github.com/boschglobal/vss-tools/pull/5. Looks good as I see it. But maybe we should merge #367 first as changing pre-commit will introduce quite a lot of changes (and thus merge conflicts)

nwesem commented 4 weeks ago

converting this back to draft for the moment as I can't get it to work the way I want, will revisit asap