obsproject / obs-plugintemplate

GNU General Public License v2.0
285 stars 133 forks source link

Run clang-format on script changes #112

Closed EZ64cool closed 1 month ago

EZ64cool commented 7 months ago

Description

As very little clang-format related code is committed to master it's hard to tell when clang-format is broken, as such testing when changes to it's scripts are detected should help find breaking changes.

Motivation and Context

A recent change (https://github.com/obsproject/obs-plugintemplate/pull/82) wasn't caught because no source files were changed, as such this remained broken for about a month. With this change, the PR would have failed the clang-format check and been caught.

How Has This Been Tested?

I've run pull requests to test this against breaking and non-breaking changes https://github.com/EZ64cool/obs-plugintemplate/actions?query=branch%3Aclang-format-validation-testing

Types of changes

Tweak (non-breaking change to improve existing functionality)

Checklist:

PatTheMav commented 7 months ago

If the intent is to force clang-format to run (and possibly fail) whenever either .clang-format|or run-format.zsh are modified, then I suppose this would be okay. Would still like a second opinion. @PatTheMav ?

That omission is on purpose - the check is for enforcing formatting on changed source code files. If only the script or formatting rules are changed, no source file has been changed and as such no violation can occur.

CI is not meant or intended to serve as a general tool to ensure all code is properly formatted - it acts on intent and in this case would only fail if the intent is to change a source file that is not properly formatted.

(In theory the action should not even be triggered in the first place, the reason it runs at all is because GitHub Actions don't have built-in support for triggers in changed files).

On larger codebases this would create even bigger issues as just updating the rules would then lead to a cascade of mandatory code changes just to get the rules updated (because everything would need to be put into the same PR/change set) and that creates far too large an impact IMO.