Consider migrating code formatting to pre-commit

[BrianPugh]

Instead of the bespoke tools/codeformat.py script, it would probably be better to move to the popular pre-commit tool to invoke all linting/formatting/static-analysis tools. Github Actions workflows would also need to be updated to invoke pre-commit.

If migrating to pre-commit sounds good, I can open up a PR.

[jimmo]

We've already done this in the main repo It works very well.

I'm not entirely sure of the history of why the codeformat.py script in this repo has so much extra logic (which the equivalent main repo script doesn't) but I suspect all that can all be removed if we're using it primarily from pre-commit. It also doesn't need the .c file handling.

(I'd like to keep tools/codeformat.py though as it's a convenient and consistent way that works in both repos)

[BrianPugh]

Any reason why we cannot ditch the codeformat.py in both repos and put the logic directly in .pre-commit-config.yaml? Consistent way to manually run in both repos would be pre-commit run --all. For example, something like the following might reach party with codeformat.py and verifygitlog.py in the main micropython repo

repos:

  - repo: local
    hooks:
      - id: uncrustify
        name: Uncrustify
        entry: uncrustify
        args: ['-c', 'tools/uncrustify.cfg', '--no-backup', '-l', 'C']
        language: system
        files: # TODO add PATHS
        exclude: # TODO add EXCLUSIONS

  - repo: https://github.com/jorisroovers/gitlint
      rev: v0.19.1
      hooks:
        - id: gitlint
          # TODO: add config here

  - repo: https://github.com/charliermarsh/ruff-pre-commit
      rev: v0.0.265
      hooks:
        - id: ruff

[jimmo]

I think that's not a bad idea.

But the simple answer is codeformat.py does more than just run uncrustify on .c files. Unfortunately we haven't been able to find a tool+config that matches the MicroPython style exactly, so codeformat.py runs some additional logic over the uncrustify output). (The problem here is MicroPython's extensive use of the preprocessor. Personally I would rather adapt our style to match a tool (preferably, clang-format) but that's a pretty drastic change. You can see the history starting with micropython/micropython#5700 and back through micropython/micropython#5691 micropython/micropython#4223 micropython/micropython#5698 micropython/micropython#5631 micropython/micropython#5671 micropython/micropython#5668 micropython/micropython#5632 micropython/micropython#5699) I guess we could find a way to move our uncrustify wrapper into its own repo and use it as a source for pre-commit though.

codeformat.py works without any additional dependencies (other than Python of course, which is already a build dependency). I guess you could argue that black and uncrustify are dependencies though, so why not pre-commit.

I do like the idea of moving to gitlint instead of our custom verifygitlog.py.

[stinos]

Any reason why we cannot ditch the codeformat.py in both repos and put the logic directly in .pre-commit-config.yaml?

Using codeformat.py I format my own C modules etc which are outside of the micropython repository. And from an editor, on one single file. Building tools using other tools just like we have ci.sh separately is just pretty convenient because it supports all usecases. So unless we have 'standard' formatting we do need our own tool, and hence separate from pre-commit.

[andrewleech]

I guess we could find a way to move our uncrustify wrapper into its own repo and use it as a source for pre-commit though.

You can have a local script as a pre-commit hook, it doesn't have to be in its own repo: https://pre-commit.com/index.html#repository-local-hooks

[jimmo]

pre-commit support added in #706 for black and ruff.

As mentioned in the PR, I have preliminary support for gitlint too, will send a PR soon.

[jonnor]

The referenced MR seems to have been merged. So I think this issue can be closed?

[BrianPugh]

I think enough movement has been made that we can close this issue.