Skip to content

Request For Comment: Add additional PR checks for formatting #274

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

Closed
wants to merge 2 commits into from

Conversation

paulbartell
Copy link
Contributor

Add some additional linters from the pre-commit project.

Specifically enable these checks on files that have changed as part of a pull-request:

  • trailing-whitespace (remove trailing whitespace from lines)
  • end-of-file-fixer (check that files end with an LF or CRLF)
  • check-yaml (link yaml files)
  • check-added-large-files (really of less usefulness after a push)
  • mixed-line-ending (reject files with a mix of LF and CRLF line endings)
  • check-case-conflict (check files with the same name on case-insensitive filesystems)
  • check-executables-have-shebangs
  • check-json (link json files)
  • check-merge-conflict (check for unresolved merge conflicts)
  • check-symlinks (check of dangling symlinks)
  • check-ast (validate python)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -0,0 +1,18 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to add the uncrustify check from github.com/pocc/pre-commit-hooks and move a copy of the formatting config to this kernel repo. https://github.com/FreeRTOS/FreeRTOS/blob/master/tools/uncrustify.cfg

@milesfrain
Copy link
Contributor

paulbartell#10 is an initial attempt to use uncrustify for formatting.

Not sure if a pre-commit hook is the best path forward, since that can be pretty annoying with unexpected formatting rewrites and having to constantly use --no-verify to suppress the hook. A better option might be to just catch this stuff in PR CI and provide an easy way for folks to run the autoformatter locally.

@paulbartell
Copy link
Contributor Author

paulbartell#10 is an initial attempt to use uncrustify for formatting.

Not sure if a pre-commit hook is the best path forward, since that can be pretty annoying with unexpected formatting rewrites and having to constantly use --no-verify to suppress the hook. A better option might be to just catch this stuff in PR CI and provide an easy way for folks to run the autoformatter locally.

The uncrustify pre-commit hook needs some configuration so that it acts as one would expect. Namely, it needs to only run on files that have changed and operate in-place without leaving backup files everywhere. I'll do some debugging on this later this week.

@milesfrain
Copy link
Contributor

The uncrustify pre-commit hook needs some configuration so that it acts as one would expect. Namely, it needs to only run on files that have changed and operate in-place without leaving backup files everywhere.

I believe that's how it currently operates by default (at least that's what I've observed in paulbartell#10 locally).

My point is that we may not want pre-commit to auto-edit files "behind the scenes" when a user simply wants to make a local commit.

@paulbartell
Copy link
Contributor Author

paulbartell commented Mar 23, 2021

The uncrustify pre-commit hook needs some configuration so that it acts as one would expect. Namely, it needs to only run on files that have changed and operate in-place without leaving backup files everywhere.

I believe that's how it currently operates by default (at least that's what I've observed in paulbartell#10 locally).

My point is that we may not want pre-commit to auto-edit files "behind the scenes" when a user simply wants to make a local commit.

For sure. It should be optional. You can always run "pre-commit" from within a git repo without installing it as a git pre-commit hook.

@codecov
Copy link

codecov bot commented Jun 22, 2021

Codecov Report

Merging #274 (994bd14) into main (56428a9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #274   +/-   ##
=======================================
  Coverage   92.13%   92.13%           
=======================================
  Files           4        4           
  Lines        1272     1272           
  Branches      342      342           
=======================================
  Hits         1172     1172           
  Misses         53       53           
  Partials       47       47           
Flag Coverage Δ
unittests 92.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56428a9...994bd14. Read the comment docs.

@Skptak Skptak closed this Sep 20, 2023
laroche pushed a commit to laroche/FreeRTOS-Kernel that referenced this pull request Apr 18, 2024
* Use unsigned types/constants where needed.

* Address MISRA 21.15 violations in FreeRTOS_Sockets.c

* Address MISRA rule violations in code (primarily Rule 2.2)

* Inline had been disabled for Coverity builds, preventing
Coverity from correctly identifying dead code; this change
removes the disabling of inline during Coverity builds.
* Added an explanation for the inline suppression of Rule
11.4 in prvSocketValid().

Co-authored-by: Aniruddha Kanhere <[email protected]>
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.

4 participants