feat: Add pre-commit

[tomadamatkinson]

Description

Many contributors have grown tired of the QA process in the repository. I don't blame them... its not ideal!! I attempted to resolve this by automatic commits but as most contributions come from forks this is not viable (See #895). This PR takes a second approach using git hooks. pre-commit adds a config file driven approach which pulls the tools needed for each check. This should help resolve the version discrepancies found with clang_format.py.

Adds pre-commit a QA tool that runs checks on commit. The clang-format check makes sure that files are formatted correct before a commit is created.

If files are not correctly formatted pre-commit will reject the commit

The formatted changes are given to the user as an unstaged change

Adding the files and created the commit will then pass the checks. As i made no change other than the format issues (which pre-commit fixed) the checks did not need to run the second time around

Note, in the future we could auto resolve copyright check issues using the same solution. Pre-commit also comes with a large selection of supported hooks so there is room for expansion with this approach

[tomadamatkinson]

I cant add you as a reviewer but @jherico I would appreciate your feedback on this approach also!

[SaschaWillems]

Just a quick question before trying this: What version of clang format will this use? The one installed locally? And if so, what's with MSVC users where clang-format isn't available via path by default?

[tomadamatkinson]

@SaschaWillems this should download the correct version. I have clang-17 locally but this check runs clang-15

[tomadamatkinson]

If we adopt this tool i will create a second PR running pre-commit run --all-files. Currently this changes quite a few as it adds whitespace at the end of a lot of files. I would also want to add the large file check and some others in that second PR

[jherico]

Looks good.

[gary-sweet]

I think having commit hooks is a good way to solve the issues we've been seeing.

I did have some issues getting pre-commit to work however. I was getting this error when trying a commit:
AttributeError: module 'virtualenv.create.via_global_ref.builtin.cpython.mac_os' has no attribute 'CPython2macOsFramework'
Note: I'm not using a Mac.

A quick search took me to this stack overflow page, and running python -m pip uninstall virtualenv resolved the issue for me (as it then picked up a different version elsewhere on the system).

I guess my concern is that problems like this could just present a different barrier to getting code into the repo. How easy would it be to use the commit hooks without another (potentially problematic) third-party tool such as pre-commit?

[tomadamatkinson]

pre-commit can also be installed in other ways. Python is likely the simplest for most users. But yeah this small hiccup can happen with python due to the many different ways python can be installed on a system.

If we used hooks without pre-commit or another tool we will run into the same issue of incorrectly installed tools. The benefit of pre-commit is its ability to pull the correct tools when needed. A general hook written by ourselves would also take a lot more time to maintain and would not be extensible

Pre-commit also does not need to be installed for a user to be able to commit. We can run pre-commit in the CI on PRs (as a check, not an auto commit). This means for users that want a smoother contribution experience, installing and using pre-commit could save them a lot of pain in the long run

I use pre-commit across many repositories and tech stacks professionally. It has hooks for most things you need and you can add local scripts just as easily (copyright fixes could be made automatic).

[SaschaWillems]

This works, and as a stop-gap this might be an option. It just feels odd, esp. if you commit from IDEs like Visual Studio, where it looks like this on commit:

[SaschaWillems]

Or could we (also) add this as e.g. a pre- or post-build event?

[tomadamatkinson]

You should be able to run pre-commit at any point. So we could automate this further than just a git hook

[SaschaWillems]

LGTM!

I tested two scenarios:

• Without pre-commit installed, where it behaves just like before
• With pre-commit installed. where it properly did a local change with clang-format applied

[marty-johnson59]

Merging - 3 approvals