Tests: Add a lint check for trailing whitespace #11005
Conversation
This adds a new CHECK_DOC check that looks for newly introduced trailing whitespace. Existing trailing whitespace (of which there is plenty!) will not trigger an error. This is written in a generic way so that new lint-*.sh scripts can be added to contrib/devtools/, as I'd like to contribute additional lint checks in the future.
maflcko
left a comment
There was a problem hiding this comment.
Concept ACK, as trailing white space causes issues with some editors that remove it automatically on save from the whole file, even from untouched lines.
contrib/devtools/lint-whitespace.sh
Outdated
| fi | ||
|
|
||
| showdiff() { | ||
| if ! git diff "${TRAVIS_COMMIT_RANGE}" --; then |
There was a problem hiding this comment.
I assume you need to set the context lines to 0 to prevent false positives?
-U0
There was a problem hiding this comment.
That doesn't hurt, although I think it's correct as is. Context lines are shown with a leading space when they have no changes, so a context line that starts with + will actually have a leading space. I'll make this change anyway for safety/clarity.
contrib/devtools/lint-whitespace.sh
Outdated
| } | ||
|
|
||
| # Do a first pass, and if no trailing whitespace was found then exit early. | ||
| if ! showdiff | egrep -q '^\+.*\s+$'; then |
There was a problem hiding this comment.
Nit: Use grep -E instead of egrep which is deprecated :-)
contrib/devtools/lint-whitespace.sh
Outdated
| fi | ||
| echo "$line" | ||
| fi | ||
| done < <(showdiff | egrep '^(diff --git |\+.*\s+$)') |
There was a problem hiding this comment.
Nit: Use grep -E instead of egrep which is deprecated :-)
|
Concept ACK |
|
@MarcoFalke @practicalswift Thanks for the suggestions, I've made these changes. |
|
If this is merged, I'll submit some whitespace-only PRs to clean up various files in the project (probably starting with markdown files, and then moving onto code if that goes well). I feel like it's not worth sending out any whitespace PRs though until this has landed, since otherwise there could be new regressions. |
|
@eklitzke Whitespace only PRs aren't likely to be accepted. Basically they disrupt other work and patches, and create rebasing issues, without providing much value. See the first point in the dev notes. "Do not submit patches solely to modify the style of existing code." We also have clang-format rules that can be used against new/modified code, which should handle formatting. |
|
@fanquake Noted. Although I did have problems with |
|
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits |
|
Can we ignore violations introduced in the directories of our imported dependencies? I'm thinking about:
Also, what about adding a similar check for introductions of new |
|
@eklitzke Are you still working on this? |
ryanofsky
left a comment
There was a problem hiding this comment.
utACK d8a7add if the the right files / directories are excluded. It looks like git has a pathspec feature that let you easily exclude paths with :! syntax.
Other things that could be done in future prs:
- Incorporate the scripted diff check
contrib/devtools/commit-script-check.shinto this linting framework instead of running it separately. - Make it possible to disable or pass options to the linter with directives in the commit message.
|
|
||
| # We can't run this check unless we know the commit range for the PR. | ||
| if [ -z "${TRAVIS_COMMIT_RANGE}" ]; then | ||
| exit 0 |
There was a problem hiding this comment.
Why exit as if the check was successful instead of showing an error?
|
Closing due to inactivity. Please ping me to get this reopened. This is up for grab by other contributors. |
1f379b1 Add tab char lint check and exclude imported dependencies (MeshCollider) dd36561 Add a lint check for trailing whitespace. (Evan Klitzke) Pull request description: This is a new attempt at #11005 Addressed nits, excluded imported dependencies, squashed the original commits, and added a test for tab characters in the *.cpp *.h *.md *.py *.sh files too as per @practicalswift suggestion Tree-SHA512: d2dfbedc8469026f39b0c63d9a71d8b8e2ed3815d69fecaabad10304d977d6345728c4c865ec7600ed539b1f7cabaa826b50312f4d2eef0a1583d4ff9024c36d
…*dash* no travis *DASH* doesn't implement this check into travis 1f379b1 Add tab char lint check and exclude imported dependencies (MeshCollider) dd36561 Add a lint check for trailing whitespace. (Evan Klitzke) Pull request description: This is a new attempt at bitcoin#11005 Addressed nits, excluded imported dependencies, squashed the original commits, and added a test for tab characters in the *.cpp *.h *.md *.py *.sh files too as per @practicalswift suggestion Tree-SHA512: d2dfbedc8469026f39b0c63d9a71d8b8e2ed3815d69fecaabad10304d977d6345728c4c865ec7600ed539b1f7cabaa826b50312f4d2eef0a1583d4ff9024c36d
This adds a new CHECK_DOC check that looks for newly introduced trailing
whitespace. Existing trailing whitespace (of which there is plenty!)
will not trigger an error.
This is written in a generic way so that new lint-*.sh scripts can be
added to contrib/devtools/, as I'd like to contribute additional lint
checks in the future.
Example of what a lint failure looks like in Travis: https://travis-ci.org/eklitzke/bitcoin/jobs/262052953