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

Tests: Add a lint check for trailing whitespace #11300

Merged
merged 2 commits into from Sep 14, 2017
Merged

Tests: Add a lint check for trailing whitespace #11300

merged 2 commits into from Sep 14, 2017

Conversation

meshcollider
Copy link
Contributor

@meshcollider meshcollider commented Sep 11, 2017

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

@fanquake fanquake added the Tests label Sep 11, 2017
@practicalswift
Copy link
Contributor

utACK 258094a

@meshcollider
Copy link
Contributor Author

Fixed a mis-copied comment in the tab character check which was referring to whitespace again


# Check if tab characters were found in the diff.
if showcodediff | grep -P -q '^\+.*\t'; then
echo "This diff appears to have added new lines with tab characters not spaces."
Copy link
Member

@sipa sipa Sep 12, 2017

Choose a reason for hiding this comment

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

This doesn't sound like correct English to me (but I'm not a native speaker myself). Use "instead of spaces" perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't think it's technically wrong but it doesn't sound quite right, yours sounds better. Fixed.

.travis.yml Outdated
@@ -46,6 +46,7 @@ install:
before_script:
- if [ "$CHECK_DOC" = 1 -a "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/commit-script-check.sh $TRAVIS_COMMIT_RANGE; fi
- if [ "$CHECK_DOC" = 1 ]; then contrib/devtools/check-doc.py; fi
- if [ "$CHECK_DOC" = 1 ]; then contrib/devtools/lint-all.sh; fi
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be guarded by TRAVIS_COMMIT_RANGE if you fail on -z $TRAVIS_COMMIT_RANGE, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not even sure if I should have changed it to fail, should it just exit quietly if there's no commit range?

Copy link
Member

Choose a reason for hiding this comment

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

Actually this should be fine, as TRAVIS_COMMIT_RANGE should never be empty according to the travis doc. (It is only empty for branches with a single commit, which we don't have)

@mess110
Copy link
Contributor

mess110 commented Sep 12, 2017

I can't manage to run the script. Am I calling it wrong?

TRAVIS_COMMIT_RANGE=50fae68d416b4b8ec4ca192923dfd5ae9ea42773...b3d6fc654770e3b4d2f82e8d77e531df9e522982 ./contrib/devtools/lint-all.sh
fatal: There is nothing to exclude from by :(exclude) patterns.
Perhaps you forgot to add either ':/' or '.' ?

I got the commit range from https://travis-ci.org/bitcoin/bitcoin/jobs/272143045

I get the same if I run

TRAVIS_COMMIT_RANGE=50fae68d416b4b8ec4ca192923dfd5ae9ea42773...b3d6fc654770e3b4d2f82e8d77e531df9e522982 git diff -U0 "${TRAVIS_COMMIT_RANGE}" -- ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/"

This works:

TRAVIS_COMMIT_RANGE=50fae68d416b4b8ec4ca192923dfd5ae9ea42773...b3d6fc654770e3b4d2f82e8d77e531df9e522982 git diff -U0 "${TRAVIS_COMMIT_RANGE}"

@meshcollider
Copy link
Contributor Author

meshcollider commented Sep 13, 2017

@mess110 my guess is that you have a different version of git which doesn't assume all files to be included if only excludes are given, but I've updated the commit to (hopefully) fix it for you

@mess110
Copy link
Contributor

mess110 commented Sep 13, 2017

Thanks, that was it indeed.

ACK b8c0cfa

@practicalswift
Copy link
Contributor

utACK b8c0cfae6d6f39fe38dcfec229f3824a5af44b29

1 similar comment
@maflcko
Copy link
Member

maflcko commented Sep 13, 2017

utACK b8c0cfae6d6f39fe38dcfec229f3824a5af44b29

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

Tested ACK. I've tested locally that both checks work, and that the src/leveldb/ exclusion also works.

It'd be helpful to also print out the @@ line showing the line number where the whitespace was introduced. Something like:

diff --git a/contrib/devtools/lint-whitespace.sh b/contrib/devtools/lint-whitespace.sh
index 5bea86a..8a065fe 100755
--- a/contrib/devtools/lint-whitespace.sh
+++ b/contrib/devtools/lint-whitespace.sh
@@ -40,0 +41,2 @@ if showdiff | grep -E -q '^\+.*\s+$'; then
+    elif [[ "$line" =~ ^@@ ]]; then
+      LINENUMBER="$line"
@@ -46,0 +49 @@ if showdiff | grep -E -q '^\+.*\s+$'; then
+        echo "$LINENUMBER"
@@ -51 +54 @@ if showdiff | grep -E -q '^\+.*\s+$'; then
-  done < <(showdiff | grep -E '^(diff --git |\+.*\s+$)')
+  done < <(showdiff | grep -E '^(diff --git |@@|\+.*\s+$)')
@@ -64,0 +68,2 @@ if showcodediff | grep -P -q '^\+.*\t'; then
+    elif [[ "$line" =~ ^@@ ]]; then
+      LINENUMBER="$line"
@@ -70,0 +76 @@ if showcodediff | grep -P -q '^\+.*\t'; then
+        echo "$LINENUMBER"
@@ -75 +81 @@ if showcodediff | grep -P -q '^\+.*\t'; then
-  done < <(showcodediff | grep -P '^(diff --git |\+.*\t)')
+  done < <(showcodediff | grep -P '^(diff --git |@@|\+.*\t)')

should do it. This only prints out the line number for the first match in each file, but I think that's ok.


# We can't run this check unless we know the commit range for the PR.
if [ -z "${TRAVIS_COMMIT_RANGE}" ]; then
echo "Cannot run lint-whitespace.sh without commit range"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add help text here explaining how to run locally:

  echo "To run locally, use:"
  echo "TRAVIS_COMMIT_RANGE='<commit range>' .lint-whitespace.sh"
  echo "eg:"
  echo "TRAVIS_COMMIT_RANGE='47ba2c3...ee50c9e' .lint-whitespace.sh"

eklitzke and others added 2 commits September 14, 2017 10:49
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.
@meshcollider
Copy link
Contributor Author

Rebased on master to fix merge conflict and addressed @jnewbery's comments, thanks!

@maflcko
Copy link
Member

maflcko commented Sep 14, 2017

re-utACK 1f379b1

@maflcko maflcko merged commit 1f379b1 into bitcoin:master Sep 14, 2017
maflcko pushed a commit that referenced this pull request Sep 14, 2017
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
@meshcollider meshcollider deleted the 201709_whitespace_lint branch September 14, 2017 09:43
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2019
…*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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 21, 2019
…*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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 2, 2020
…*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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 4, 2020
…*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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 4, 2020
…*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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 10, 2020
…*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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 10, 2020
…*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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 10, 2020
…*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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
…*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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants