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

update pre-commit hooks #363

Closed
wants to merge 3 commits into from
Closed

Conversation

nwesem
Copy link
Contributor

@nwesem nwesem commented May 31, 2024

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

Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
@erikbosch
Copy link
Collaborator

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
Copy link
Collaborator

MoM: Please Review

Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
@nwesem
Copy link
Contributor Author

nwesem commented Jun 18, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

nwesem commented Jun 25, 2024

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
Copy link
Collaborator

erikbosch commented Jun 25, 2024

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 boschglobal#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 nwesem marked this pull request as draft June 25, 2024 11:37
@nwesem
Copy link
Contributor Author

nwesem commented Jun 25, 2024

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

Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
@erikbosch
Copy link
Collaborator

MoM: Waiting for other PRs to be merged

@sschleemilch
Copy link
Collaborator

Would propose something like this #393

@SebastianSchildt
Copy link
Collaborator

done by #393

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.

4 participants