Add first version of pre-commit

[davidrudlstorfer]

Add first version of more exhaustive pre-commit hook

[davidrudlstorfer]

@isteinbrecher this would be ready for review

This PR introduces the first version of a more exhaustive pre-commit hook for MeshPy - and simply executes it in the check-code action

One thing needs to be changed within this PR:

We need to adjust the github workflow to skip the no-commit-to-branch, i.e., that no one can commit to main, on the main branch to ensure this check is not executed on the main branch - otherwise it will fail.

In the future I see a few small improvements which I did not want to add to this PR to not add to many changes

• We could (and should) enforce absolute imports, i.e., meshpy.rotation.blablub rather than ..rotation.blablub
• We could (and in my opinion should) enforce the docstrings, currently we have 96% coverage. This would improve the docu which could be created from the docstrings
• We could further enforce the docstring style, i.e. args to be consistent across the code base
• We could add type hinting - I think we briefly talked about this and you had some points against that?
• We should enforce that each source code file contains the license header

A pre-commit hook now looks like this

Check for added large files..........................................................Passed
Check Python files for parse errors..................................................Passed
Check files for case conflicts on case-insensitive filesystems.......................Passed
Check that no code is placed prior to docstrings.....................................Passed
Check that executables have shebangs.................................................Passed
Check for illegal Windows filenames..............................(no files to check)Skipped
Check valid JSON syntax..........................................(no files to check)Skipped
Check for files that contain merge conflict strings..................................Passed
Check that shebang scripts are executable............................................Passed
Check valid TOML syntax..............................................................Passed
Check valid XML syntax...........................................(no files to check)Skipped
Check valid YAML syntax..............................................................Passed
Check for newline at end of file.....................................................Passed
Check for debugger imports...........................................................Passed
Check to not commit to main..........................................................Passed
Check for trailing whitespaces.......................................................Passed
Run bandit (security linter for python)..............................................Passed
Run clang-format (C/C++ formatter)...................................................Passed
Run docformatter (formatter for docstrings)..........................................Passed
Run interrogate (linter for docstrings)..............................................Passed
Run mypy (static type checker for python)............................................Passed
Run removestar (remove wildcard imports).............................................Passed
Run ruff (linter for Python).........................................................Passed
Run ruff (formatter for Python)......................................................Passed
Run xml-formatter................................................(no files to check)Skipped
Run yamlfmt..........................................................................Passed

[davidrudlstorfer]

I've now also changed the check-code option to not execute the no-commit-to-branch on the main branch

I've also changed the check-code option to just use an Ubuntu image? Prior these checks were done within the 4C image although this is not needed?

[isteinbrecher]

Thanks a lot @davidrudlstorfer for these changes! I will have a look, but it might take until next week.

Regarding your questions:

We could (and should) enforce absolute imports, i.e., meshpy.rotation.blablub rather than ..rotation.blablub

Sounds good to me

We could (and in my opinion should) enforce the docstrings, currently we have 96% coverage. This would improve the docu which could be created from the docstrings

Sounds good to me

We could further enforce the docstring style, i.e. args to be consistent across the code base

I would like to see how that looks like, but in general I am for something like this

We could add type hinting - I think we briefly talked about this and you had some points against that?

I remember we talked about this and you mentioned, that this only makes sense if it is done throughout the code base (and type are also enforced?). I am for adding type hints (the cosserat_curve module already comes with quite some), I am just not sure if we want/can do it everywhere.

We should enforce that each source code file contains the license header

We already have that, the test tests/testing_header.py (should probably be moved to the code checks instead of being run inside the test suite)

[isteinbrecher]

And yes, the default ubuntu image is fine for the code checks

[isteinbrecher]

Thanks a lot @davidrudlstorfer for taking a look into this, this will definitely put the code quality of MeshPy to another level. I left some comments regarding the configuration. I have two more questions:

• Is it possible to have one commit that applies all the automatically done stuff and one that adds the manually required changes for everything to pass?
• For practical purposes, if I want to commit something and I get an error message in the pre commit hook, do I get a command that I can use to apply the suggested changes or are they applied automatically, how does that work?

[davidrudlstorfer]

Thanks a lot @davidrudlstorfer for taking a look into this, this will definitely put the code quality of MeshPy to another level. I left some comments regarding the configuration. I have two more questions:

• Is it possible to have one commit that applies all the automatically done stuff and one that adds the manually required changes for everything to pass?

Unfortunately, I've combined both changes into a single commit and I would prefer not to split them up again as it was a lot of manual work 🥶 if you're ok with it I would leave it as is

• For practical purposes, if I want to commit something and I get an error message in the pre commit hook, do I get a command that I can use to apply the suggested changes or are they applied automatically, how does that work?

That depends on the specific hook - all formatting changes are applied completely automatically if they fail, i.e., formatting any sort of code (C++, Python) or formatting docstrings.

Code related problems are not automatically adjusted - there the user has to intervene manually, i.e., mypy and bandit and the tools provide an error where type problems may arise or sometimes also show one or more solutions on how to solve the problem

[isteinbrecher]

Thanks for the explanations @davidrudlstorfer. I tried to "split" your first commit by first applying just the automatic changes from the pre-commit hooks and then resetting the state to your commit, I think that should have worked (see the branch on my fork https://github.com/isteinbrecher/meshpy/tree/add_pre_commit_hook). Can you have a short look if those are your manual changes (in the commit 16253c4) and if so push that commit structure to this PR? (The actually code in the final state is exactly the same).

[davidrudlstorfer]

@isteinbrecher I've now rebased everything onto main - as there were also some changes which were necessary to the cosserat module.

I've also split up everything into logical commits and also split the automatic and manual changes as you suggested.

I encounter one problem which I was not able to solve directly - maybe you could have a brief look:

The code check now fails with a problem in the new cosserat module:

meshpy/cosserat_curve/cosserat_curve.py:391: error: Unsupported operand types for <= ("list[float]" and "float")  [operator]
meshpy/cosserat_curve/cosserat_curve.py:393: error: Unsupported operand types for >= ("list[float]" and "float")  [operator]
meshpy/cosserat_curve/cosserat_curve.py:398: error: Unsupported operand types for - ("list[float]" and "float")  [operator]

(see actions output)

Unfortunately, I was not able to quickly test this behaviour as it was not triggered within the testing pipeline (I simply wanted to exit once this part is reached)

Is this really wrong code or is there another problem here?

EDIT: It is tested 🤦🏻 I did not use pytest, therefore, the cosserat tests did not execute

[davidrudlstorfer]

@isteinbrecher so the problem within the cosserat curve was that the variable arc_length was previously initialized as a list, then reset to a float. MyPy uses the first initialisation of a variable => therefore it was wrong

if you are happy with these changes I would combine the last two commits into the manual changes

[isteinbrecher]

I adapted the changes in cosserat_curve and pushed them to your branch (sorry for force pushing without warning to your repo, I didn't think that would work 😅).

[isteinbrecher]

I will have a look at the other changes now, I think it is best if we combine the commits once everything is resolved.

[isteinbrecher]

Thanks a lot @davidrudlstorfer for the effort put into this. I really like the new look and feel. I had a look at all the changes, and adapted the ones where I had some issues directly - now I am asking for your review on those. If you agree with them, I think we can squash the relevant commits together and merge this!

[davidrudlstorfer]

@isteinbrecher also big thanks to the effort you put into the review 🙏🏻

I think this lays a good foundation to further improve the code quality within the repo and for future work.

Thanks for improving the manual code changes necessary to comply with the new pre-commit hook and associated changes.

From my side everything is alright.

As a last step I would squash the last 6 commits into the manual changes and then I would be happy to merge 🚀

[isteinbrecher]

Thanks again @davidrudlstorfer! Let's get this merged, great to see MeshPy improving!

[davidrudlstorfer]

@isteinbrecher unfortunately you have to merge as I am unable to merge 😅

[isteinbrecher]

I am also not allowed to merge, since I added commits and therefore my review is not valid... But I will make use of
:)

[isteinbrecher]

One thing I just saw in the test report, we don't actually check any xml files:

Check valid XML syntax...........................................(no files to check)Skipped
...
Run xml-formatter................................................(no files to check)Skipped

all the vtk, vtu, ... are xml based. Should we add them to the checks?