Integrate clang-format - Githubissues

pret / pokeplatinum

Decompilation of Pokémon Platinum

177 stars 61 forks source link

Integrate clang-format #176

Closed lhearachel closed 2 months ago

lhearachel commented 5 months ago

Usage

For convenience, a format.sh script is included; the following command will format the full source tree according to rules specified in .clang-format:

./format.sh

A pre-commit git hook is also included, along with instructions for contributors on how to integrate it into their commit workflow.

Next Steps

clang-format does not support renaming constants, variables, etc. We will need a separate clang-tidy integration if we want to standardize that process.

lhearachel commented 5 months ago

This PR is nonessential, and functionally on-hold; I'll revisit it here and there as I have time to do so.

mid-kid commented 4 months ago

Spotted this in my notifications. I don't have much to comment on regarding the style, only the implementation:

I think it's awkward to invoke the build system for this, as it's not building anything, rather modifying the build tree. It's better as a maintenance script, not something to run as part of any build.
Regarding auto-formatting on PR, I'd rather it give a CI warning/error (preferably warning) if the autoformatter detects formatting changes in the changed lines. This allows for the possibility to evaluate the formatting changes and possibly choose to override the formatter's decision. Kind of like linux' checkpatch.pl

lhearachel commented 4 months ago

I think it's awkward to invoke the build system for this, as it's not building anything, rather modifying the build tree. It's better as a maintenance script, not something to run as part of any build.

I don't disagree, but Meson gives it to us for free out of the box, and it saves us having to write and maintain a script to traverse/walk our source tree. Maybe it should just be a separate ./format.sh script that serves to invoke the ninja target?

Regarding auto-formatting on PR, I'd rather it give a CI warning/error (preferably warning) if the autoformatter detects formatting changes in the changed lines. This allows for the possibility to evaluate the formatting changes and possibly choose to override the formatter's decision. Kind of like linux' checkpatch.pl

What about a git hook?

mid-kid commented 4 months ago

Meson gives it to us for free out of the box

Oh, I didn't know this. Quite a questionable feature, but if it needs no extra work then I don't mind.

What about a git hook?

I'm not sure, I've never used a git hook in a multiperson project, but I think this'd be most useful to newbie contributors, so it should be default and hard to overlook.