Proposal: Replace code-formatter-helper with pre-commit

[antonkesy]

I’d like to propose migrating from code-formatter-helper to a well-known solution: pre-commit.

Thank you in advance for checking out the change. Just tell me if you need more information to open a discussion or if this is not something you are interested in. I'd love to hear some feedback ❤️

• Replaces ./Scripts/reformat.sh with:
  • pre-commit for only changed files
  • pre-commit run -a for all existing files
• Add black and isort for Python
• Developers can add pre-commit hooks locally to automatically run the formatter before committing

While this is a bigger change for developers, it eliminates the need to manage and fix code-formatter-helper.
The CI is unchanged, except that it now runs on GitHub servers instead of self-hosted runners, for now.

All Python file changes here are just pre-commit running black and formatting, but it would be straightforward to create a separate PR for that if desired.

Example outputs

To show some example outputs, I just misformatted something and ran pre-commit/tried to commit.
C++:

clang-format.............................................................Failed
- hook id: clang-format
- files were modified by this hook
isort....................................................................Passed
black....................................................................Passed

Python:

clang-format.............................................................Passed
isort....................................................................Passed
black....................................................................Failed
- hook id: black
- exit code: 123

error: cannot format Scripts/ClassifyCPU.py: Cannot parse: 16:0: v8_1Mandatory = ["atomics", "asimdrdm", "crc32"]

Oh no! 💥 💔 💥
20 files left unchanged, 1 file failed to reformat.

Additionally, it would be possible to, instead of just silently reformatting the files, show the exact line (dry-run):

clang-format.............................................................Failed
- hook id: clang-format
- exit code: 1

Source/Tools/LinuxEmulation/Thunks.h:14:8: error: code should be clang-formatted [-Wclang-format-violations]
public:
       ^


isort....................................................................Passed
black....................................................................Passed

[Sonicadvance1]

If you delete this file then it breaks my workflow. As I run it prior to committing anything.

ryanh@ryanh-TR5000:/mnt/Work/Work/work/FEXNew$ cat clang_format.sh
#!/bin/sh
sh -c "CLANG_FORMAT=clang-format-19 ./Scripts/reformat.sh . > /dev/null 2>&1"

Ensuring the user is formatting their code correctly and leaving the onus on them is desired.

[antonkesy]

I've expected that this would happen.

There would be 2 possible solutions:

1. You could change your clang_format.sh to run pre-commit instead of reformat.sh and the behaviour would be the same (in addition to also run Python formatting if required):

• pre-commit (only formats changed files)
• pre-commit run -a (for all files)

2. Install pre-commit (pre-commit install), and every time before you commit, pre-commit and therefore clang-format are run. This is the reason pre-commit exists: not to have to run these kinds of scripts manually.

But it would be understandable if this were to change too much in trusted workflows.

[Sonicadvance1]

The CI is unchanged, except that it now runs on GitHub servers instead of self-hosted runners, for now.

That's not going to fly, we're never going to use GitHub's runners.

[antonkesy]

The CI is unchanged, except that it now runs on GitHub servers instead of self-hosted runners, for now.

That's not going to fly, we're never going to use GitHub's runners.

Understood. I guess it would be easy to switch from GitHub runners to the self-hosted ones. Pre-commit only requires Python and installs the rest itself. Therefore, all the Python environment setup from the code-formatter-helper-workflow could be avoided.

[antonkesy]

Not sure if just changing this to:

    runs-on: [self-hosted, X64]

would be enough to have it working on the self-hosted solution. I'm not familiar with the setup.

[Sonicadvance1]

@pmatos should probably look at this because I don't know enough about the code formatting stuff to know if this is worth using.

[pmatos]

I must say that besides adding more dependencies on the side of FEX, I am not sure what advantages this brings. If you push failing to format, it's detected by CI. And since you can't force people to use pre-commit, you need that workflow anyway. It doesn't seem this is going to ease things in any way.

Am I missing something here?

[antonkesy]

I must say that besides adding more dependencies on the side of FEX, I am not sure what advantages this brings

I wouldn't call it addding more dependencies, rather replacing dependencies and reducing own maintenance burden.

If you push failing to format, it's detected by CI. [...] you need that workflow anyway

Agree, but when I see a project using pre-commit, I can quickly check if this covers formatting too. I didn't know that FEX had a custom helper script, since the Scripts directory is full of other scripts too and might be overlooked.

For me, I see the adoption as the only true problem/change with adapting pre-commit, and would say the pros outweigh the cons.
Just seeing that the migration commit +30/-847 lines changed is a strong indicator of reduced maintenance burden.

Cons:

• Existing workflows must adapt:
  Developers need to install pre-commit and replace usage of ./Scripts/reformat.sh with pre-commit (or install the hook to run it automatically).

• Requires developers to install pre-commit:
  But the same could be argued for any tool currenlty used, since developers must install the exact version of clang-format, or setup a Python environment if they want to match the CI.

Pros

• Standardized, well-maintained tooling:
  Replaces 2 custom scripts with a short central configuration file. pre-commit is widely used and maintained by a large community, reducing maintenance burden on FEX.

• Cross-language formatting in one step:
  pre-commit would handle both C++ and Python formatting in a single command. No need to create and maintain separate scripts for each language.

• Consistent local and CI behavior:
  Same configuration for both; no need to specify clang-format versions manually or maintain separate scripts for local vs full CI runs.

• Plug & Play: Zero manual environment setup:
  pre-commit installs and pins its own tool versions (clang-format, black, isort). No need to install the right Python packages or match the CI environment.

• Automatic pre-commit hooks:
  The whole point of pre-commit is to run formatting automatically before commits. No need to remember to run formatting scripts manually and wait for CI to catch issues.

[neobrain]

We appreciate the effort and interest, but we're happy with the current setup. Also please consider revisiting your use of LLMs, since many would consider throwing in AI slop as rude and unhelpful (even more so if it isn't disclosed).

(others, feel free to re-open if there's a desire to integrate this)

[antonkesy]

Also please consider revisiting your use of LLMs, since many would consider throwing in AI slop as rude and unhelpful (even more so if it isn't disclosed).

Okay, wow no idea where this accusation comes from now ...
Most would consider it very rude to be accused of using LLMs after taking that much time and effort ... (I know, hard to understand, but ppl still can and enjoy writing text by hand)
Definitely a nice way to keep new interested volunteers for sure ... Feels bad for the bug fixes I've prepared already

But hey, I've got the drift how stuff works here .. Thanks

Rant mode off

[neobrain]

Okay, wow no idea where this accusation comes from now ...

This "Pro & Con" format with bullet lists is a fairly typical style of LLM output, particularly with the boldface summary in each bullet item. You see people post such output in an attempt to be helpful, which as a maintainer is cumbersome and wasteful of our time. Apologies if I got unlucky by calling out the one comment where no LLM was actually used.

LLM or not though, the current setup works for us regardless and we have no desire to change it.