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

Code format workflow #465

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

MuhammadTahaNaveed
Copy link
Member

@MuhammadTahaNaveed MuhammadTahaNaveed commented Dec 29, 2022

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).
image
image

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/h files because rules defined in .clang-format are only for C/Cpp/h files.

Resolves #380

- this workflow will run on push or pull request to master branch.
- the workflow will fail if any PR or push is not abiding by the code
  standards defined in .clang-format
- You can also run "pre-commit" on your repo locally.
- This is because rhaschke/upload-git-patch-action@main is not allowed
  to be used in apache/age.
@MuhammadTahaNaveed
Copy link
Member Author

@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.

@MuhammadTahaNaveed
Copy link
Member Author

@jrgemignani Issue is fixed. All checks are passing.

@JoshInnis JoshInnis closed this Dec 31, 2022
@JoshInnis JoshInnis reopened this Dec 31, 2022
@apache apache deleted a comment from jrgemignani Dec 31, 2022
@jrgemignani
Copy link
Contributor

jrgemignani commented Jan 3, 2023

@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.

@jrgemignani
Copy link
Contributor

@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/

@MuhammadTahaNaveed
Copy link
Member Author

MuhammadTahaNaveed commented Jan 5, 2023

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.

image

@jrgemignani
Copy link
Contributor

@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
Copy link
Member Author

@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.

@jrgemignani
Copy link
Contributor

jrgemignani commented Jan 5, 2023

@MuhammadTahaNaveed We will probably need to do some cleanup of the repository before we finally implement this.

@MuhammadTahaNaveed
Copy link
Member Author

MuhammadTahaNaveed commented Jan 5, 2023

@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.

@jrgemignani
Copy link
Contributor

@MuhammadTahaNaveed That is true, but we want to be sure that the changes made, or flagged, are acceptable.

@MuhammadTahaNaveed
Copy link
Member Author

MuhammadTahaNaveed commented Jan 12, 2023

@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
    image
    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).

@MuhammadTahaNaveed
Copy link
Member Author

One more thing

  • Clang format does not provide any options to ensure naming conventions and stuff like that.

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.

Ensure code standards/rules using GitHub Actions
3 participants