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

REPO: Add clang pre-commit workflow #466

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

Conversation

mkmer
Copy link
Collaborator

@mkmer mkmer commented Jan 24, 2025

Activation of clang-format checking when a PR is created. clang-format will only check the new code, existing code can stay incorrectly formatted until it is "touched".
I have run clang-format on the entire repo - results can be seen here: https://github.com/mkmer/app_rpt/tree/activate-clang-format. I believe it matches the Asterisk code guidelines (unless I made a mistake) - and moving forward will help us stick with the Asterisk guidelines.

Individuals can run clang-format locally with git clang-format -i --style=file:./dev/.clang-format --diff --staged to format locally before committing.

Copy link
Member

@InterLinked1 InterLinked1 left a comment

Choose a reason for hiding this comment

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

This change is wrong: mkmer@9b86265#diff-e9c04babdfa10caa4961bbcc4d26b4d2b254adcd13d5c6d3a40dfa342f455653L661

I think this is also wrong: mkmer@9b86265#diff-e9c04babdfa10caa4961bbcc4d26b4d2b254adcd13d5c6d3a40dfa342f455653R701

Ditto for mkmer@9b86265#diff-e9c04babdfa10caa4961bbcc4d26b4d2b254adcd13d5c6d3a40dfa342f455653R924

I haven't looked at everything but those are a few things to start with. I like the idea but I think this will need to be fine tuned a bit before we've got it exactly right. I think there is also an indent command in the Asterisk wiki and the output could be cross-checked with that, though I'm not sure how faithful that is either.

@mkmer
Copy link
Collaborator Author

mkmer commented Jan 25, 2025

Sorry - the links are not connecting to the lines of code you are referencing :( I'm not sure why.
My goal was to match indent rules just using clang-format.
I'll take some time with indent in a different branch. I suspect our code base doesn't match it either.

@mkmer mkmer force-pushed the github_clang_workflow branch from 0765c54 to c27c215 Compare January 25, 2025 01:34
@mkmer
Copy link
Collaborator Author

mkmer commented Jan 25, 2025

I found a few more "fixes" - struct formats were goofy, and { 0,1,2 } was changing to {0,1,2} -> both have been addressed in the configuration.
It will only be enforced on new commits (only the code modified). This full run is more of a "did I miss any odd format".

@InterLinked1
Copy link
Member

I found a few more "fixes" - struct formats were goofy, and { 0,1,2 } was changing to {0,1,2} -> both have been addressed in the configuration. It will only be enforced on new commits (only the code modified). This full run is more of a "did I miss any odd format".

I wouldn't be opposed to doing a pass on the existing code, but we'd need to make sure it's 100% right first before doing so.

I'm curious, can this also check for if conditions without the optional braces, in the case of a single statement? That seems like a common issue we identify in reviews.

@mkmer
Copy link
Collaborator Author

mkmer commented Jan 25, 2025

Yes, one of the options was to add braces, remove braces, etc. I have it set to add braces for all if statements (and other's).

The workflow will fail and complain about error (assuming they didn't run it before the commit), the developer will need fix the error (either manual edit, or running the tool locally).
We can also just merge a PR if a test fails -> these tools just help new dev's get it "right" and reviewers to not have to find this stuff :)

@mkmer mkmer force-pushed the github_clang_workflow branch 2 times, most recently from 7eefcc8 to 8e00f13 Compare January 26, 2025 14:00
.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved

- name: Run clang-format check
id: clang_check
uses: mkmer/clang-format-action@v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

mkmer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reviewed a TON of other actions, they all wanted to format the code and commit it to the repo.
I modified one to just exit 1 with a "change".

Copy link
Collaborator

Choose a reason for hiding this comment

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

My comment here was asking why we have uses: mkmer/clang-format-action@v1 vs. adding our own workflow here with this PR?

Note: if we need to use this for more than one repo, then we should look at adding ours asl3-workflows repo.

.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
@mkmer mkmer force-pushed the github_clang_workflow branch from 8e00f13 to ab6c049 Compare January 26, 2025 14:33
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.

3 participants