-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
ci: Add pre-commit #2636
ci: Add pre-commit #2636
Conversation
I had a test run with some of the tools but the changes are too severe. I would like to have the tools optional and I'd like to see only occasional change suggestions. I propose to take this much slower, test and tune one tools after the other before adoption. |
Thanks for checking it out. I've changed the clang-format hook to manual. I think the whitespace adjustments (end-of-line/file) and shell script formatting steps are reasonable - but I can also change those to manual. |
I have added a git workflow as well. I'll make sure that the git workflow passes once the pending "issues"/PRs are decided on - there are some PRs that fix some isues.
|
I was working on getting cpplint installed ... Will let you know when that is ok. |
As far as I am concerned, the next step would be to merge this PR which does not reports any "issue" any more for the current codebase. Next, the other PRs enabling other hooks would ideally be merged. Currently only the 'cpplint' hook reports issues that were not resolved yet. |
I have updated all the PR's related to the setup of pre-commit in the project. I did not propose the fix for the clang-format issue that appeared in the recent code changes. With regards to the cpplint annotations that still exist - these either need to be fixed or ignored to make that useful in ci. The annotations concern the use of long (not time_t related) and recommendations to use snprintf. I also added a tool I created in ci that converts the notifications from the tools in to a checkstyle compatible xml which is used by cs2pr to annotate the code in github. 10 annotations for errors and 10 for warnings are shown in the Summary of the workflow run: https://github.com/merbanan/rtl_433/actions/runs/7410578345 . From there you can click to see the annontation in context: So IMHO all of this is usefull to help maintain code quality on incoming requests - but for that it needs to be added to the master branch of course. It's best to first accept the "pre-commit" PR and then the other ones which enable the relevant pre-commit hooks. |
I propose to add pre-commit.
This PR only enables the hooks that do not detect issues.
I created other PRs that enable other hooks one by one.
Except for cpplint, the issues reported by these other hooks are now fixed and merged.
pre-commit can be run locally to check the code and make corrections before committing.
This PR also adds what is needed to run pre-commit in the CI-workflow.
It helps detects bugs and improve code quality.
Local usage: