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

ci: Add pre-commit #2636

Closed
wants to merge 1 commit into from
Closed

ci: Add pre-commit #2636

wants to merge 1 commit into from

Conversation

mdeweerd
Copy link
Contributor

@mdeweerd mdeweerd commented Sep 24, 2023

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:

# install pre-commit on the system
pip install pre-commit

# run certain hooks
pre-commit run -a codespell
pre-commit run -a cpplint

# Setup git to run pre-commit on every commit (to run in local clone).
pre-commit install

@zuckschwerdt
Copy link
Collaborator

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.

@mdeweerd
Copy link
Contributor Author

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.
The spelling and python code message are easy to fix and fairly limited - but again, those can be set to manual as well.

@mdeweerd
Copy link
Contributor Author

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.

  • cpplint complains about 'long' : "Use int16/int64/etc, rather than the C type long". Should these messages be ignored in existing code or should long be changed to a (u)int64_t or (u)int32_t type.

@mdeweerd
Copy link
Contributor Author

I was working on getting cpplint installed ... Will let you know when that is ok.

@mdeweerd mdeweerd changed the title Add pre-commit ci: Add pre-commit Oct 23, 2023
@mdeweerd
Copy link
Contributor Author

Except for codespell (fix proposed in #2730),
all pre-commit hooks resulting in changes have been set to "manual" so that they can be enabled one by one.

So this PR will become green once rebased to the merge of #2730.

@mdeweerd
Copy link
Contributor Author

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.
Merging this will help detect issues in future merges (with the enabled hooks).

Next, the other PRs enabling other hooks would ideally be merged.

Currently only the 'cpplint' hook reports issues that were not resolved yet.

@mdeweerd
Copy link
Contributor Author

mdeweerd commented Jan 4, 2024

I have updated all the PR's related to the setup of pre-commit in the project.
The PRs that fixed spelling and line endings allows the corresponding pre-commit targets to pass again.

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:
https://github.com/merbanan/rtl_433/pull/2734/files#annotation_16766607303

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.

@mdeweerd mdeweerd closed this by deleting the head repository Feb 29, 2024
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.

2 participants