[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!
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [MERGED] Pre-commit hooks

Post by abdullah »

openBrain wrote: Mon Apr 03, 2023 7:39 pm Do you think that on Debian/Ubuntu, it works as well to install with `apt install pre-commit` ?
Generally better use the package manager than pip. :)
Just for confirmation, I am on Ubuntu 22.04, I have installed pre-commit from the official repo. It appears to work ok so far.

I have just PR-ed clang-formatting changes for PlaneGCS and Sketcher App.

I am holding changes for Gui pending the other PR with the making pop-ups respect the intrusive/non-intrusive configuration in preferences, but I intend to make the shift to clang and pre-commit before release. This way we start the next development cycle without formatting issues.

I am opening the files before running clang to check for inline comments beyond the 100 characters. In some cases the best option is the previous line, others the next line, yet others I found it best to move the comment elsewhere.

For *PyImp.cpp files, there are two includes (.h .cpp). These need to be separated one line to avoid reordering. Reordering them leads to compilation error (the .h must be before the .cpp). A blank line between them avoid clang from reordering (reordering is done per block).

The rest goes quite smoothly so far.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [MERGED] Pre-commit hooks

Post by abdullah »

With this:
https://github.com/FreeCAD/FreeCAD/pull/9662

The Sketcher WB will be clang-format/black/pre-commit framework compliant.

The directories listed in .pre-commit-config.yaml apply recursively. If one adds src/Mod/MyWB/, the pre-hook will apply to all subdirectories.

As a recount of this new experience, I can say that I have browsed the diff of the result. Sometimes comment lines are merged when adjusting lines. Sometimes one may think the result is not very stylistic. Most of the times the tool does an acceptable work. I have fixed some of these and re-run the tool.

Now, I would be interested in:
1. asking developers, specially those contributing to the Sketcher WB, to use the pre-commit framework.
2. have some clang-format / black check failing in CI (only for the Sketcher WB and the other parts already in the framework.

@openBrain

Any plans?
User avatar
chennes
Veteran
Posts: 3879
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: [MERGED] Pre-commit hooks

Post by chennes »

abdullah wrote: Thu May 25, 2023 1:41 pm 2. have some clang-format / black check failing in CI (only for the Sketcher WB and the other parts already in the framework.
We haven't actually implemented it yet, but the basic process will be for CI to also run the same precommit script, except not actually make any changes, just verify that no changes would be made. We could go so far as to outright reject a PR that does not pass. @openBrain what do you think?
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [MERGED] Pre-commit hooks

Post by openBrain »

chennes wrote: Thu May 25, 2023 2:05 pm We haven't actually implemented it yet, but the basic process will be for CI to also run the same precommit script, except not actually make any changes, just verify that no changes would be made. We could go so far as to outright reject a PR that does not pass. @openBrain what do you think?
That shouldn't be much of a problem I think.
We have some questions to answer :
* Do we pass it only on changed files ? We could pass it on all concerned files but that doesn't seem fair to reject a PR because a file that wasn't touched isn't correct
* How do we architecture it ? In Lint workflow ? In another dedicated workflow ?
BTW, I don't think we can really "reject" PR with CI. We can just mark it as failing the CI. To reject it (for real) we would need to implement a server-side hook, which probably isn't so easy to reliably do in GH. ;)
User avatar
chennes
Veteran
Posts: 3879
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: [MERGED] Pre-commit hooks

Post by chennes »

I think yes to only looking at changed files: that's how pre-commit operates, so it's consistent. And my inclination is to give it its own workflow, but I don't feel strongly about that.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [MERGED] Pre-commit hooks

Post by openBrain »

chennes wrote: Thu May 25, 2023 3:06 pm I think yes to only looking at changed files: that's how pre-commit operates, so it's consistent. And my inclination is to give it its own workflow, but I don't feel strongly about that.
Depends how severe we want to be. :)
* Add in "Lint" : this is the shortest (in running time) workflow. It already has a runner and downloads sources. So almost everything is there already. The main drawback probably is it is considered a "not mandatory" workflow to pass by many when merging. So it is a bit of a loose introduction.
* Add in "Prepare" : more manual operations as probably the best is to only download "manually" changed files and analyse them. Not a big deal though. Where it's very different is that failing "Prepare" makes the build workflows to be skipped and not to run. So it will be close to a direct no-go for merging. This is the more severe way to introduce.
* Add another parallel workflow : very convenient and readable for maintainer. However that will be another runner and another source download which from resource management PoV isn't very nice. ;)
User avatar
chennes
Veteran
Posts: 3879
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: [MERGED] Pre-commit hooks

Post by chennes »

I personally am perfectly happy with a no-go solution -- ultimately we want all developers running precommit, and we're still doing a "soft" intro, where the config file is only checking some selected areas of the code with the strict formatting checks. I defer to your expertise on resource management.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [MERGED] Pre-commit hooks

Post by abdullah »

Thank you! :)

The prepare solution sounds good to me. It also saves building resources until a PR complies. I see that as a bonus.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [MERGED] Pre-commit hooks

Post by openBrain »

OK, I have something working but found a limitation (was actually already here, but found when testing with large changesets).
The current implementation only can deal with changesets counting less than 300 files.

Is it fine limiting the CI to a maximum change of 299 files for a single PR/push, and make it early failing without even trying to build if 300+ files are changed in the patch ?
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [MERGED] Pre-commit hooks

Post by abdullah »

openBrain wrote: Sat May 27, 2023 12:26 pm OK, I have something working but found a limitation (was actually already here, but found when testing with large changesets).
The current implementation only can deal with changesets counting less than 300 files.

Is it fine limiting the CI to a maximum change of 299 files for a single PR/push, and make it early failing without even trying to build if 300+ files are changed in the patch ?
I think there will be PRs with more than 300 files.

For example, one change I would like to promote is to move the Console framework from printf syntax (%d) to format syntax ({}). This needs to be done at once for all FreeCAD. The reason is that when Console is changed to accept the format syntax, all calls to console will need to be changed to operate properly. See:
https://github.com/FreeCAD/FreeCAD-tran ... issues/226

[I just realised that the ticket should be in FreeCAD and not in FreeCAD-translations, my bad, sorry]

I am not good a git statistics, but I managed to write this:

Code: Select all

for i in `git --no-pager log --pretty=oneline -n 100 | awk -F' ' '{print $1}'`; do echo $i `git show --stat $i | tail -1`; done
So this gets the number of files changes per commit, for the last 100 commits (feel free to change the -n 100 to get more). There are several commits with more than 300 changes.

Or better, since a date, and showing only those with more than 300 files:

Code: Select all

for i in `git --no-pager log --pretty=oneline --since="2022-05-27" | awk -F' ' '{print $1}'`; do echo $i `git show --stat $i | tail -1`; done | awk -F' ' '{if($2>300)print$2}'
497
497
545
545
766
915
915
1771
901
886
917
690
679
441
532
532
492
708
1011
1391
322
498
1225
1286
2601
307
697
805
348
434
1028
you can complement that with wc for a single line:

Code: Select all

for i in `git --no-pager log --pretty=oneline --since="2022-05-27" | awk -F' ' '{print $1}'`; do echo $i `git show --stat $i | tail -1`; done | awk -F' ' '{if($2>300)print$2}' | wc -l
31
So, 31 commits had more than 300 files affected since 27 May 2022.
Post Reply