add formatting and linting python in pre-commit

[m-fila]

BEGINRELEASENOTES

• Added ruff pre-commit hook for formatting and linting python files
• Reformatted and fixed warnings in python files
• Added cmake targets ruff and ruff-format to lint and format python code

ENDRELEASENOTES

Linting and formatting done with ruff which should be compatible with black, flake8 and pylint while having a single configuration file. ruff is already in the stack

CI added as local hook

Selected rules ["F", "E", "W", "PLE", "PLW", "PLC"] from flake8 (pyflakes "F", pycodestyle errors "E" and warnings "W") and pylint (errors "PLE", warnings "PLW" and conventions "PLC")
I'd also consider adding some of the D1 checks for docstrings but there seems to be only a few docstrings now 🙃

The codebase seems to use mixed 2-space and 4-space indent, now the default 4-space is used in formatter

Should I also add cmake targets as in #257?

Closes #364

[m-fila]

No ruff in the LCG releases? 👀

[tmadlener]

Should I also add cmake targets as in #257?

We either adapt that to run the same checks as this, or we have to include flake8 and pylint in this PR as well. Alternatively, we close that and include the changes here.

[m-fila]

Added cmake targets here so #257 can be closed if this is merged

[tmadlener]

Very nice. Thanks :)

[jmcarcell]

What about including formatting as part of Key4hepConfig.cmake? For python ruff is very fast so it won't change much, clang-format for every file I don't know 🤔

[tmadlener]

I like the idea. We would probably then also need a pre-commit setup that checks in all repositories. In the end the targets are only defined but not run by default, so it would be a somewhat easy way for users to fix formatting issues that are flagged by pre-commit.

[m-fila]

Hmm pre-commit config is just a single file and if we want to centrally manage it then the projects are not supposed to modify it, right?
So if we decide that we don't want for instance clang-tidy in pre-commit everywhere then adding clang-tidy just to selected repos would be a problem?
Similar problem with ruff config, ruff format and lint are in the same file so if we want to have managed ruff config with just format then it wont be possible to add lint to it. Also the config then should be probably moved from recommended pyproject.toml to ruff specific otherwise it won't be possible to modify pyproject.toml?

[tmadlener]

I think some of the configuration files simply can't be managed centrally, or at least not for all packages. It's not entirely clear to me where to draw the line, but e.g. pyproject.toml is not something I would manage centrally, since there is a lot of project specific metadata that goes into there.

[m-fila]

So should we centrally manage just the cmake target with ruff format but no linting target, pre-commit config, ruff config? Ruff can run on the defaults without any config file, the only difference between the defaults and the config here is the target-version = "py38" vs target-version = "py310"

[tmadlener]

Maybe python related targets need to go into a separate .cmake file? I am not entirely sure if all the repositories will need it in the end. Additionally, Gaudi option files are not easily lintable, I think, since most of the things are dynamically populated.

I would leave the pre-commit config on a per repository basis at least. (The github action can then again be distributed centrally). For the linting, I am not entirely sure, in principle we should apply the same rules for all repositories. However, that would then mean that we would need to go the way of ruff specific config files, right? Because pyproject.toml is definitely repository specific, IMHO.

[tmadlener]

Should we merge this still before the tag (tomorrow)? Or do we postpone this until later and conclude the discussion first?

[m-fila]

I think there is no hurry for this one
I just realized I missed there is a black config in k4FWCore with different line width (99). I think I'll change it here to 99 too for some consistency

[tmadlener]

Looks good to me, thanks. If nobody complains, I will merge this later today.

[tmadlener]

I was just about to say we should add this. Nice :)

[andresailer]

Should this be py311? cf. https://github.com/key4hep/k4geo/pull/398/files#diff-b3236d4cb260ca6acce083493fcdc42816f732fc3d5aefb263a3c3fe9db4f81f

[m-fila]

I tried to be compatible with the black config from k4FWCore https://github.com/key4hep/k4FWCore/blob/main/pyproject.toml since at that moment that was the only python formatting config out there (except for spack I think)

[andresailer]

Then let's put py310 in k4geo (@tmadlener)?

[m-fila]

Updating to py311 here and in k4FWCore and reformatting doesn't change any files. We have python 3.11 in the nightlies. I think we could just update and have py311 everywhere

I can also copy the extra formatting options from k4geo (it's just being explicitly setting the values same as the default right?)

[tmadlener]

it's just being explicitly setting the values same as the default right?

Yes. Specifically the defaults that make ruff work / format the same as black.

[tmadlener]

I think we could just update and have py311 everywhere

I would be in favor of that.

[m-fila]

Updated here

[andresailer]

No ruff in the LCG releases? 👀

There will in the next one: https://lcginfo.cern.ch/pkg/ruff/

(Hopefully happening Monday)

[tmadlener]

We could also switch over to use a key4hep stack, since that is what we are doing on other repositories, I think. Using an LCG stack was done historically, because we could more easily get an LLVM based stack (for clang-format), but that is now also available from a key4hep stack