Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run clang-format on script changes #112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EZ64cool
Copy link
Contributor

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 (#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:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

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.
@RytoEX RytoEX requested a review from PatTheMav January 29, 2024 22:39
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@PatTheMav
Copy link
Member

PatTheMav commented Jan 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants