FreeRTOS-Kernel icon indicating copy to clipboard operation
FreeRTOS-Kernel copied to clipboard

Request For Comment: Add additional PR checks for formatting

Open paulbartell opened this issue 4 years ago • 5 comments

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.

paulbartell avatar Mar 06 '21 00:03 paulbartell

https://github.com/paulbartell/FreeRTOS-Kernel/pull/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.

milesfrain avatar Mar 22 '21 06:03 milesfrain

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.

paulbartell avatar Mar 23 '21 00:03 paulbartell

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.

milesfrain avatar Mar 23 '21 17:03 milesfrain

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.

paulbartell avatar Mar 23 '21 18:03 paulbartell

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.

codecov[bot] avatar Jun 22 '21 20:06 codecov[bot]