apache
Code format workflow
What I did
Created a workflow to ensure that any files touched or created by PR or commit is meeting the code formatting standard/rules defined in .clang-format. I used pre-commit to scan the code and generate automatic warnings on code style violations.
Why I did it
There were some code formatting standards defined in clang-format.5 ( renamed to .clang-format in this PR ) but there is no workflow to ensure that any new PR or commit is actually meeting that standard. This workflow will allow the reviewer to focus on architecture of a change while not wasting time with trivial style nitpicks. Contributor can review the warnings generated by pre-commit in case the check fails by seeing the details(refer img).
Additional Information
- As this github action will run on the files touched, this may also introduce warnings on the code that is not modified by the contributor, this can be because of existing violations of code formatting rules in code.
- This workflow will only check code formatting violation on
C/Cpp/hfiles because rules defined in .clang-format are only forC/Cpp/hfiles.
Resolves #380
@MuhammadTahaNaveed Why was this closed?
I don't know why the check failed when I created the PR. This workflow was running fine on my fork. That's why I closed it. I will research why it is failing and reopen it soon.
@JoshInnis Please do not delete comments made by people other than yourself. If you have an issue with a comment, please discuss it with that individual to have them correct or remove their own comment. Unless, it is spam, of course.
@MuhammadTahaNaveed
These changes will only flag, with an error, the violations?
Did you make sure that the clang file is current with our coding standards? https://age.apache.org/contribution/guide/
These changes will only flag, with an error, the violations?
Yes
Did you make sure that the clang file is current with our coding standards? https://age.apache.org/contribution/guide/
I did not cross check because of the following statement in documentation[refer img]. But I can certainly do that and update the clang-format file if some guidelines written in documentation are missing in clang-format file.
@MuhammadTahaNaveed Yeah, that is a bit misleading. The clang file has not really been updated in a while and might be missing a few coding rules.
Additionally, we need to think about how we will roll this out. There is the possibility that many files may already have some minor violations due to changes in the coding styles or just minor overlooked cases.
@MuhammadTahaNaveed Yeah, that is a bit misleading. The clang file has not really been updated in a while and might be missing a few coding rules.
Okay, then I will cross check the rules in clang-format with the guidelines written in documentation and add rules if some are missing.
Additionally, we need to think about how we will roll this out. There is the possibility that many files may already have some minor violations due to changes in the coding styles or just minor overlooked cases.
Yes, I thought about this before submitting the PR. The way I implemented the workflow makes sure that validation checks only run on the files touched/modified or created by the contributor. However this may also introduce some warnings on the legacy code that was not following the rules in those specific files.
@MuhammadTahaNaveed We will probably need to do some cleanup of the repository before we finally implement this.
@MuhammadTahaNaveed We will probably need to do some cleanup of the repository before we finally implement this.
It can be done automatically by running pre-commit locally. Precommit will automatically find the violations and modify the code according to the rules in .clang-format.
@MuhammadTahaNaveed That is true, but we want to be sure that the changes made, or flagged, are acceptable.
@jrgemignani I have cross checked the rules defined in .clang-format with the ones defined in documentation. Most of the rules are current with documentation. However I had some confusions.
- Documentation says to indent case labels in switch-case blocks (refer img), but in clang-format, it is set to false
IndentCaseLabels: false
I have seen in code as well that case labels are not idented. - Documentation says
If all the bodies of if/else statement contain a single line, omit braces.but I don't think their is any style option listed in clang style options documentation to ensure this (as far as I have seen).
Other then that, .clang-fromat looks good to me.
P.S sorry for the delay (just got free from exams).
One more thing
- Clang format does not provide any options to ensure naming conventions and stuff like that.