[MERGED] Pre-commit hooks

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
chennes
Veteran
Posts: 3868
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

[MERGED] Pre-commit hooks

Post by chennes »

Many projects, particularly large ones, have a set of pre-commit hooks that they ask developers to install and run before submitting code. These are pieces of code that run on each changed file in a commit and can either modify the files directly, or cancel the commit if some criteria are not met. While projects like Qt have very rigorous checks, I think that FreeCAD can benefit from a simpler set that can help reduce some of the burden on Contributors and Maintainers so we can focus on code, and not formatting, negligible whitespace changes, etc. The advantage of running these tools as pre-commit hooks is that developers can use any IDE they like, with any formatting options they like: during the process of making a commit an external tool runs to normalize the code formatting without any regard for the vagaries of different IDE implementations.

I propose that we take this in phases:
  1. We ask that Contributors install pre-commit, a Python package that aids in the management of pre-commit hooks
  2. We ask that Contributors install the black and clang-format Python modules (using black 23.x and clang-format 16.x)
  3. Within the FreeCAD git repository we create .pre-commit-config.yaml to store our default pre-commit configuration. We begin with:
    • trailing-whitespace (removes trailing whitespace)
    • clang-format, configured to run only on directories in a specific list, whose Maintainers have already applied clang-format on all existing files. Initially that list of directories is empty.
    • black, configured to run only on directories in a specific list, whose Maintainers have already applied Black on all existing files. Initially that list of directories is AddonManager and FEM.
  4. As they have the time and opportunity to do so, Maintainers run clang-format and/or Black as appropriate on the code they are responsible for, in coordination with the other developers working on that code. The commits introducing those formatting changes are added to .git-blame-ignore-revs.
  5. Once a given subdirectory has been reformatted, it is added to the list in .pre-commit-config.yaml so that the pre-commit hook runs on changed files in that subdirectory from that point forward.
If this process is successful we can consider applying additional hooks (for example, to format CMake files, or to run codespell, or linters) -- there are many available: https://pre-commit.com/hooks.html

We can also write our own: for example to ensure that copyright headers are in place, or that the copyright on FCStd files is LGPL2-compatible.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
User avatar
bernd
Veteran
Posts: 12849
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland
Contact:

Re: Pre-commit hooks

Post by bernd »

Where does the reformatting for example BLAC for python code really takes place?

As I undterstand it normally on the local machnie of the developer. If a developer misses this it takes place on github befor the commit is merged or rebased into master and the changes are inside the commit. But what on lets say 10 commits ... This would not work, as it will give conflicts from one commit to the next. Or ist code without the pre-hook just not accepted.

For sure I am missing something ...
wmayer
Founder
Posts: 20203
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Pre-commit hooks

Post by wmayer »

using black 23.x and clang-format 16.x
On Ubuntu 22.04 the available versions are black 21.12 and clang-tidy-15. Will they work, too?
openBrain
Veteran
Posts: 9031
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Pre-commit hooks

Post by openBrain »

bernd wrote: Wed Mar 29, 2023 10:13 am As I undterstand it normally on the local machnie of the developer.
Yes. At the moment a 'git commit' command is issued.
If a developer misses this it takes place on github befor the commit is merged or rebased into master and the changes are inside the commit.
Nope. It can't happen on server side. What should happen in GH is that it checks push is compliant, and rejects or warns if it's not the case.
User avatar
bernd
Veteran
Posts: 12849
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland
Contact:

Re: Pre-commit hooks

Post by bernd »

openBrain wrote: Wed Mar 29, 2023 10:34 am
bernd wrote: Wed Mar 29, 2023 10:13 am As I undterstand it normally on the local machnie of the developer.
Yes. At the moment a 'git commit' command is issued.
If a developer misses this it takes place on github befor the commit is merged or rebased into master and the changes are inside the commit.
Nope. It can't happen on server side. What should happen in GH is that it checks push is compliant, and rejects or warns if it's not the case.
thanks. That way it perfectly makes sense to me ...

I like this pre-commit hooks. Sounds good to me.
User avatar
bernd
Veteran
Posts: 12849
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland
Contact:

Re: Pre-commit hooks

Post by bernd »

chennes wrote: Wed Mar 29, 2023 4:29 am ...
[*] black, configured to run only on directories in a specific list, whose Maintainers have already applied Black on all existing files. Initially that list of directories is AddonManager and FEM.
...
In the regard of FEM. Where do you have this from? Here: https://github.com/FreeCAD/FreeCAD/blob ... entions.md This line ...

Code: Select all

For new code if someone would like to use a code formatter black should be used
The formatting I introduced in FEM is similar to black but it is not black. AFAIK ATM no file of FEM really uses black. Years back I made some tests to totally switch to black for FEM (at a time when I was the only one makeing real dev in FEM) but never decided to do.
User avatar
chennes
Veteran
Posts: 3868
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Pre-commit hooks

Post by chennes »

bernd wrote: Wed Mar 29, 2023 12:49 pm The formatting I introduced in FEM is similar to black but it is not black.
Oh, OK, I was just misremembering then. So we start with just the Addon Manager, and grow from there. Sorry!
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
User avatar
chennes
Veteran
Posts: 3868
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Pre-commit hooks

Post by chennes »

wmayer wrote: Wed Mar 29, 2023 10:19 am On Ubuntu 22.04 the available versions are black 21.12 and clang-tidy-15. Will they work, too?
I had thought that by using a pip-installed version we could all use the latest, but the important thing is that especially for clang-format we have to pick a version, and then make our .clang-format file work with that version. For Black it's much less important because Black doesn't really have any options (that's its whole point). We may eventually have to specify that it be run in a way that it doesn't wrap text strings (because it will break our translations), but that option is still experimental and not enabled by default.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
User avatar
bernd
Veteran
Posts: 12849
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland
Contact:

Re: Pre-commit hooks

Post by bernd »

chennes wrote: Wed Mar 29, 2023 1:35 pm
bernd wrote: Wed Mar 29, 2023 12:49 pm The formatting I introduced in FEM is similar to black but it is not black.
Oh, OK, I was just misremembering then. So we start with just the Addon Manager, and grow from there. Sorry!
But as it is similar it would be possible to jump in for FEM. But it would need to be done carefully with all active FEM devs. I would help with this once this has started.
User avatar
chennes
Veteran
Posts: 3868
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Pre-commit hooks

Post by chennes »

OK, here's a proposed pre-commit configuration: https://github.com/FreeCAD/FreeCAD/pull/9140
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
Post Reply