global: Prune trailing whitespace. #13777
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13777 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 161 161
Lines 21084 21084
=======================================
Hits 20739 20739
Misses 345 345 ☔ View full report in Codecov by Sentry. |
|
Code size report: |
|
Thanks, this is a reasonable thing to do. I always try to remove any trailing space when I review/merge PRs. But, I suggest to leave the third-party code alone, ie everything in |
9df5e33 to
827c321
Compare
|
Done. Updated the commit message to reflect that these directories are avoided intentionally. |
The CI yells when there are trailing spaces in the code. |
That's why this is still a draft, rather than something I would push to be merged. It might be less troublesome to let this cleanup happen naturally, but still useful to surface where trailing whitespace occurs. (I arrived at this PR entirely out of curiosity after experimenting with linting some other codebases.)
I guess only if it's new code or on a line that has otherwise been changed? |
827c321 to
df00fe3
Compare
|
As per my sleuthing on #13763, here are a list of open PRs which I believe add trailing whitespace:
|
|
I do not recall at which time the check for trailing whitespace in code files was added to the CI, but since then it should only slip though in documentation files or commit messages. Maybe in makefiles as well. |
|
Thanks @Gadgetoid! Agree it's best if we can merge this PR and #13763 together to minimise churn. |
| @@ -3,35 +3,35 @@ | |||
| // | |||
| // MACRO and Function prototypes for TI-RTOS and Free-RTOS API calls | |||
| // | |||
| // Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com/ | |||
| // | |||
There was a problem hiding this comment.
This simplelink is third-party code. But I guess if we are going to add CI to check for trailing spaces, then probably it's easier to just remove these as well.
There was a problem hiding this comment.
if we are going to add CI to check for trailing
Isn't that already done. I have seen some complaints of the code formatting checker about trailing space.
There was a problem hiding this comment.
If it's third party code, then ports/cc3200/simplelink/oslib/osi.h is unlikely to be changed outside of total replacement? Thus it wont trigger the whitespace checks. Is it best left untouched?
afaik the only whitespace enforcement is via uncrustify, and just running that across the whole codebase results in yet another huge list of small code consistency changes- 6140ce1 (edit link updated to remove some third party code, also oof there are a lot of strange macro rewrites in there)
There was a problem hiding this comment.
and just running that across the whole codebase results in yet another huge list of small code consistency changes
You must be running it wrong, because the macro indent/dedent changes should not be there. You need to run tools/codeformat.py.
There was a problem hiding this comment.
I was running:
find . -name *.c -not -path "./lib/*" -not -path "./ports/cc3200/FreeRTOS/*" -not -path "./drivers/cc3100/*" | xargs uncrustify -c tools/uncrustify.cfg -lC --replace --no-backup
But on closer inspection it looks like tools/codeformat.py has code (fixup_c() ) to then undo these erroneous changes.
I tried running codeformat after directly running uncrustify and received a much smaller set of changes:
Weirdly if I just run something like:
find . -name *.c -not -path "./lib/*" -not -path "./ports/cc3200/FreeRTOS/*" -not -path "./drivers/cc3100/*" | ./tools/codeformat.py -v -c -f
I get a long list of files that have ostensibly been considered and changed, but no actual changes... it was always my assumption that codeformat.py only picked up files changed in the last commit, but I can't actually find a mechanism for that.
Poking a little further, it looks like all of my changes are to files that are NOT in PATHS and not in EXCLUSIONS˘, eg: drivers/bus/softspi.corexamples/natmod/btree/btree_c.c`.
I should probably raise another PR to consider these...
There was a problem hiding this comment.
PR raised here, since there are a few minor but salient issues that shouldn't be lost in this nested chat - #14046
c78bcf7 to
b270998
Compare
Prune trailing whitespace across the whole project*, this is done automatically with: grep -IUrl --color "[[:blank:]]$" --exclude-dir=.git --exclude=*.exp |\ xargs sed -i 's/[[:space:]]*$//' * Skip third-party code in lib/ and drivers/cc3100/. * Skip generated code in bluetooth_init_cc2564C_1.5.c * Preserve command output whitespace in docs, eg: docs/esp8266/tutorial/repl.rst Signed-off-by: Phil Howard <phil@gadgetoid.com>
b270998 to
8f801f3
Compare
|
Rebased, and added details of the additional exceptions to the commit message for posterity. |
|
Merged in dda9b9c |
Prune trailing whitespace across the whole project, this is done automatically with:
Raised as a draft because this touches 113 files and may break things so I do not expect it to be merged as is, but I think we should be cognisant of these... issues. I've avoided .exp files, but much of the remaining diffs are still near inscrutable.
Though GitHub will filter out whitespace from diffs if you ask nicely- https://github.com/micropython/micropython/pull/13777/files?diff=unified&w=1
If there's any interest in this kind of tidyup and this PR isn’t suitable for merging, I would recommend individual authors using some variation of the above command or editor config to clean up files as they are otherwise modified.