Warn (don't fail!) on spelling errors. Fix typos reported by codespell.

practicalswift commented at 1:40 pm on August 13, 2018: contributor

Check for common misspellings using codespell.
Fix recently introduced typos reported by codespell.

fanquake added the label Tests on Aug 13, 2018

practicalswift force-pushed on Aug 13, 2018

fanquake commented at 2:07 pm on August 13, 2018: member

Travis failure:

033.63s$ test/lint/lint-all.sh
1test/lint/lint-all.sh: line 21: test/lint/lint-spelling.sh: Permission denied
2^---- failure generated from test/lint/lint-spelling.sh

practicalswift force-pushed on Aug 13, 2018

in .travis.yml:150 in c505b76d0d outdated

142@@ -143,11 +143,11 @@ jobs:
143         BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --enable-werror"
144     - stage: lint
145       env:
146-      sudo: false
147       cache: false
148       language: python
149       python: '3.6'
150       install:
151+        - travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq codespell

ken2812221 commented at 3:10 pm on August 13, 2018:

Can use

0addons:
1  apt:
2    packages:
3    - codespell

to avoid sudo

practicalswift commented at 3:29 pm on August 13, 2018:

Oh that is much cleaner! Thanks!

practicalswift force-pushed on Aug 13, 2018

DrahtBot commented at 3:25 pm on August 13, 2018: member

#13998 (Scripts and tools: gitian-build.py improvements and corrections by hebasto)
#11911 (Free CDBEnv instances when not in use by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 3:42 pm on August 13, 2018: member

Does this mean we need to manually add words that are not recognized to the spell checker? If we do this, I think this certainly needs maintenance instructions.

in test/lint/lint-spelling.sh:13 in ab54964811 outdated

 8+
 9+export LC_ALL=C
10+
11+EXIT_CODE=0
12+EXCLUDE_REGEXP="(src/example.cpp:.*example-false-positive-word) "
13+SPELLING_ERRORS=$(codespell -d -q 3 $(git ls-files -- ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/" ":(exclude)depends/" ":(exclude)doc/release-notes/" ":(exclude)src/qt/locale/" ":(exclude)test/functional/test_framework/__init__.py" ":(exclude)test/lint/lint-spelling.sh") | grep -vE "${EXCLUDE_REGEXP}")

Empact commented at 5:26 pm on August 13, 2018:

nit: using the long form options here would be more self-documenting: --disable-colors --quiet-level=

practicalswift commented at 9:03 pm on August 13, 2018:

Good point! Now fixed.

in src/rpc/net.cpp:248 in ab54964811 outdated

247-            "\nTo disconnect by nodeid, either set 'address' to the empty string, or call using the named 'nodeid' argument only.\n"
248-            "\nArguments:\n"
249+            "\n" "Immediately disconnects from the specified peer node.\n"
250+            "\n" "Strictly one out of 'address' and 'nodeid' can be provided to identify the node.\n"
251+            "\n" "To disconnect by nodeid, either set 'address' to the empty string, or call using the named 'nodeid' argument only.\n"
252+            "\n" "Arguments:\n"

Empact commented at 5:28 pm on August 13, 2018:

Why are these changes necessary? There are plenty more leading \n in this file.

practicalswift commented at 9:04 pm on August 13, 2018:

The old Ubuntu packaged version didn’t understand \n and thus flagged “nTo” as a typo. That seems to have been fixed in the more recent pip installed version.

in .travis.yml:153 in ab54964811 outdated

146@@ -147,6 +147,10 @@ jobs:
147       cache: false
148       language: python
149       python: '3.6'
150+      addons:
151+        apt:
152+          packages:
153+          - codespell

Empact commented at 5:29 pm on August 13, 2018:

nit: codespell recommends installation via pip: https://github.com/codespell-project/codespell

practicalswift commented at 9:04 pm on August 13, 2018:

Good point! Now fixed

Empact commented at 5:31 pm on August 13, 2018: member

Concept ACK

practicalswift force-pushed on Aug 13, 2018

practicalswift commented at 9:19 pm on August 13, 2018: contributor

@laanwj No not at all :-) codespell is based on a very comprehensive list of commonly misspelled words (see the codespell GitHub project for details). Words that are not recognised as known errors are simply ignored by codespell.

False positives – words recognised as errors by codespell but where we disagree with the classification for some reason – can be handled by simply adding them to test/lint/lint-spelling.ignore-words.txt.

To get rid of all false positives in the project only four entries are needed in the ignore file which is tiny considering that the Bitcoin project contains over 50 000 unique words :-)

0$ wc -l < test/lint/lint-spelling.ignore-words.txt
14
2$ git grep "" | tr A-Z a-z | sed "s/[^a-z]/ /g" | tr " " "\n" | grep "[a-z]" | sort | uniq | wc -l
355786

practicalswift renamed this:
~~lint: Add spell check linter (codespell). Fix typos reported by codespell.~~
lint: Check for common misspellings using codespell. Fix typos reported by codespell.
on Aug 13, 2018

in src/qt/paymentserver.h:80 in d8725c0209 outdated

76@@ -77,7 +77,7 @@ class PaymentServer : public QObject
77     // to read from the file specified in the -rootcertificates setting,
78     // or, if that's not set, to use the system default root certificates.
79     // If you pass in a store, you should not X509_STORE_free it: it will be
80-    // freed either at exit or when another set of CAs are loaded.
81+    // freed either at exit or when another set of CA:s are loaded.

MarcoFalke commented at 10:15 pm on August 13, 2018:

why?

Empact commented at 6:43 am on August 14, 2018:

Yeah I would add these to test/lint/lint-spelling.ignore-words.txt rather than use : to separate them.

in src/qt/coincontroldialog.cpp:188 in d8725c0209 outdated

184@@ -185,7 +185,7 @@ void CoinControlDialog::buttonSelectAllClicked()
185                 ui->treeWidget->topLevelItem(i)->setCheckState(COLUMN_CHECKBOX, state);
186     ui->treeWidget->setEnabled(true);
187     if (state == Qt::Unchecked)
188-        coinControl()->UnSelectAll(); // just to be sure
189+        coinControl()->DeSelectAll(); // just to be sure

MarcoFalke commented at 10:16 pm on August 13, 2018:

Should probably be a scripted diff

practicalswift commented at 7:53 am on August 14, 2018:

Good point! Now fixed using a scripted-diff :-)

jnewbery commented at 2:45 pm on August 30, 2018:

Is unselect really not ok? https://en.oxforddictionaries.com/definition/unselect.

practicalswift commented at 3:01 pm on August 30, 2018:

@jnewbery I found this: https://english.stackexchange.com/questions/18465/unselect-or-deselect

I’m happy to revert this specific change and add an exclude instead if this change is controversial :-)

jnewbery commented at 3:21 pm on August 30, 2018:

Both seem fine to me, but I don’t care strongly.

laanwj commented at 7:19 am on August 14, 2018: member

I’m… not convinced about this.

I think running this once in a while is good, just to see if there’s any really awkward misspellings, but running this in travis and failing the tests just because of a misspelled word in a comment seems overkill.

As I’ve said before, many times, linters should be added cautiously, there to prevent real problems resulting in bugs.

practicalswift force-pushed on Aug 14, 2018

practicalswift commented at 7:47 am on August 14, 2018: contributor

@laanwj The reason I wrote this linter was your comment in #9962 where you described that your preference was to catch typos at review time so that they never get merged in the first place, and that typo fixes should be done without involving the maintainers if possible. Or so I understood it at least :-)

From #9962 (comment):

In principle it is good to fix comment typos, but my preference too would be to either catch them at the source (review the PR and find them) so that they never get merged in the first place or bunch them up into something like a “fix typos” pull every few weeks.

From #9962 (comment):

[…] This allows live cooperation on fixing typos without involving the maintainers at every step.

Also @fanquake requested an automated solution in #10419:

Im curious. What tool are you using to find these? I assume it’s being run in some automated way on the repository when new changes are merged? Why not merge the open pull requests instead, catch the typos/incorrect includes etc etc and then advise changes in the PRs while they are still open?

practicalswift force-pushed on Aug 14, 2018

scravy commented at 8:27 am on August 15, 2018: contributor

Maybe it would make sense to have this run as a separate check (as in github check – https://developer.github.com/v3/checks/runs/ ). Especially with things like an AppVeyor build for MSVC (https://github.com/bitcoin/bitcoin/pull/13964#issuecomment-412852328, which already is a different check) it might be handy to have this split out of the other builds.

It would need to be hosted somewhere though I guess.

practicalswift commented at 8:05 am on August 23, 2018: contributor

@laanwj Can you clarify your position? :-)

Instead of exiting with a failure if new typos are introduced in a submitted PR we could perhaps just let codespell print the identified typos to the console as part of the linting process (exit 0 instead of exit 1). Sounds like a good compromise?

DrahtBot added the label Needs rebase on Aug 27, 2018

practicalswift force-pushed on Aug 27, 2018

practicalswift commented at 2:51 pm on August 27, 2018: contributor

Now only warns in case of spelling errors by writing identified spellings to standard output (with exit 0 instead of exit 1). That way spelling errors will be noticed quickly without blocking Travis. Should be a good compromise hopefully :-)

Also rebased!

Example output:

0$ test/lint/lint-spelling.sh; echo $?
1src/streams.h:532: signficant  ==> significant
2
3False positives above? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
40

practicalswift force-pushed on Aug 27, 2018

practicalswift commented at 3:15 pm on August 27, 2018: contributor

This can be seen in action on https://travis-ci.org/bitcoin/bitcoin/jobs/421142979 – it correctly identifies the misspelled word “signficant” which was introduced in fe943f99bf0a2bbb12e30bc4803c0337e3c95b93. That commit was part of #12254 (“BIP 158: Compact Block Filters for Light Clients”) which merged a day ago. FWIW, it was a extremely thoroughly reviewed PR with many reviewers but only codespell noticed the typo :-)

Output:

 0…
 1$ set -o errexit; source .travis/lint_06_script.sh
 2Running script for: 52bc0698ca0b27cd24f6f31b335495d641fd58cf
 3sed -i 's/UnSelect/DeSelect/g' src/qt/coincontroldialog.cpp src/qt/sendcoinsdialog.cpp src/wallet/coincontrol.h
 4OK
 5src/crypto/ctaes in HEAD currently refers to tree 1b6c31139a71f80245c09597c343936a8e41d021
 6src/crypto/ctaes in HEAD was last updated in commit 8501bedd7508ac514385806e191aec21ee978891 (tree 1b6c31139a71f80245c09597c343936a8e41d021)
 7subtree commit 003a4acfc273932ab8c2e276cde3b4f3541012dd unavailable: cannot compare
 8src/secp256k1 in HEAD currently refers to tree af619602c243e0d8fbd5934f375faa4aedb4ca6e
 9src/secp256k1 in HEAD was last updated in commit fd86f998fcfd25d823d67a2920814e22445655f9 (tree af619602c243e0d8fbd5934f375faa4aedb4ca6e)
10subtree commit 0b7024185045a49a1a6a4c5615bf31c94f63d9c4 unavailable: cannot compare
11src/univalue in HEAD currently refers to tree a2dbaf30021c0a31eecc213cd84d600273d333b6
12src/univalue in HEAD was last updated in commit a570098021be6a7b9f4589300ea655ae4633628e (tree a2dbaf30021c0a31eecc213cd84d600273d333b6)
13subtree commit 51d3ab34ba2857f0d03dc07250cb4a2b5e712e67 unavailable: cannot compare
14src/leveldb in HEAD currently refers to tree f8cdc77167be86194f98d0412baf4b89d3d5abe6
15src/leveldb in HEAD was last updated in commit ec749b1bcdf2483b642fb51d635800e272c68ba6 (tree f8cdc77167be86194f98d0412baf4b89d3d5abe6)
16subtree commit 524b7e36a8e3bce6fcbcd1b5df09024283f325ba unavailable: cannot compare
17Args used        : 181
18Args documented  : 186
19Args undocumented: 0
20set()
21Args unknown     : 5
22{'-zmqpubhashtx', '-zmqpubhashblock', '-zmqpubrawtx', '-promiscuousmempoolflags', '-zmqpubrawblock'}
23* Checking consistency between dispatch tables and vRPCConvertParams
24src/streams.h:532: signficant  ==> significant
25^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
26The command "set -o errexit; source .travis/lint_06_script.sh" exited with 0.
27…

Pretty sweet :-)

practicalswift renamed this:
~~lint: Check for common misspellings using codespell. Fix typos reported by codespell.~~
Warn (don't fail!) on spelling errors. Fix typos reported by codespell.
on Aug 27, 2018

DrahtBot removed the label Needs rebase on Aug 27, 2018

practicalswift force-pushed on Aug 30, 2018

practicalswift commented at 1:45 pm on August 30, 2018: contributor

Rebased and fixed the typos that codespell found in the newly introduced documentation.

codespell catched the “account” typo first reported by @promag in PR #14023 post-merge: #14023 (review)

in src/dbwrapper.cpp:81 in 6ace30141c outdated

77@@ -78,7 +78,7 @@ static void SetMaxOpenFiles(leveldb::Options *options) {
78     // do not interfere with select() loops. On 64-bit Unix hosts this value is
79     // also OK, because up to that amount LevelDB will use an mmap
80     // implementation that does not use extra file descriptors (the fds are
81-    // closed after being mmaped).
82+    // closed after being mmap:ed).

jnewbery commented at 2:44 pm on August 30, 2018:

I’ve never seen a colon used in this way. Why not mmap’ed?

jnewbery commented at 2:55 pm on August 30, 2018: member

Apart from the unselect->deselect change, I think the spelling corrections are fine.

Concept ACK on the linting, although I haven’t reviewed the code. I’d lean toward having the linter fail on spelling errors rather than just warn (which will no doubt be ignored).

practicalswift force-pushed on Aug 30, 2018

practicalswift commented at 3:07 pm on August 30, 2018: contributor

@jnewbery Thanks for the encouraging review! PR updated – please re-review :-)

If this PR is merged I’ll look if there is some smart way to use Travis console output folding or similar to make the spelling warnings more prominent in the Travis interface.

laanwj commented at 9:39 am on August 31, 2018: member

I’d lean toward having the linter fail on spelling errors rather than just warn (which will no doubt be ignored).

I still strongly disagree with this, I don’t think we should pile up linters for cosmetic issues.

The original idea of them was to flag serious issues that can result in bugs.

Failing the tests on some supposed spelling error in a comment really, goes too far.

practicalswift commented at 1:55 pm on August 31, 2018: contributor

@laanwj What about enabling it on a warning level (as the PR is currently formulated) instead of error level? Could that be a good compromise? :-)

jnewbery commented at 2:15 pm on August 31, 2018: member

I still strongly disagree with this

ok, I won’t rehash the debate that we’ve all had in countless other PRs already :)

My point was merely that warnings tend to get ignored, so in general I think they’re less useful than errors. I don’t have any strong opinion on this PR though.

laanwj commented at 3:22 pm on August 31, 2018: member

Yes, warning is ok with me

Though I think it would be best if one of us runs this periodically, for example for every major release, and fixes the jarring typos. I don’t think this is something that necessarily needs to run continuously.

practicalswift force-pushed on Sep 3, 2018

practicalswift commented at 5:04 pm on September 3, 2018: contributor

@jnewbery When thinking about it again I agree regarding “deselect” vs “unselect”. Now added it to the ignore list instead and removed that commit. Please re-review :-)

Sjors commented at 8:16 am on September 4, 2018: member

Concept ACK for warnings. Maybe Drahbot can point out typos. In addition we could fix the ones we missed as part of the release process.

utACK 4a98785 (though maybe squash the first and last commit)

practicalswift force-pushed on Sep 4, 2018

practicalswift commented at 9:05 am on September 4, 2018: contributor

@Sjors Squashed. Please re-review :-)

Sjors commented at 9:16 am on September 4, 2018: member

utACK b5ee29e460dc36066bac68e041073100c0eca20e

Fix typos reported by codespell ada356208e

lint: Add spell check linter (codespell) f8a81f73ac

in src/qt/coincontroldialog.cpp:444 in b5ee29e460 outdated

440@@ -441,7 +441,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
441     for (const auto& out : model->wallet().getCoins(vCoinControl)) {
442         if (out.depth_in_main_chain < 0) continue;
443 
444-        // unselect already spent, very unlikely scenario, this could happen
445+        // deselect already spent, very unlikely scenario, this could happen

fanquake commented at 9:26 am on September 4, 2018:

If unselect is in the ignore list, why is this being changed?

practicalswift commented at 11:12 am on September 4, 2018:

Good point. Fixed!

practicalswift force-pushed on Sep 4, 2018

practicalswift commented at 11:12 am on September 4, 2018: contributor

@Sjors @fanquake Updated to address @fanquake’s comment. Please re-review :-)

MarcoFalke commented at 8:15 pm on September 4, 2018: member

utACK f8a81f73ac4fee82488806c2b384b36e8ee2acdf

in test/lint/lint-spelling.ignore-words.txt:2 in f8a81f73ac

0@@ -0,0 +1,6 @@
1+cas
2+hights

Empact commented at 8:32 pm on September 4, 2018:

I don’t find hights in current master

practicalswift commented at 7:45 am on September 5, 2018:

@Empact

0$ git grep -i hights
1contrib/linearize/linearize-data.py:        self.highTS = 1408893517 - 315360000
2contrib/linearize/linearize-data.py:                os.utime(self.outFname, (int(time.time()), self.highTS))
3contrib/linearize/linearize-data.py:                    os.utime(self.outFname, (int(time.time()), self.highTS))
4contrib/linearize/linearize-data.py:        if blkTS > self.highTS:
5contrib/linearize/linearize-data.py:            self.highTS = blkTS

:-)

Empact commented at 10:08 pm on September 5, 2018: member

utACK ada3562 could squash

MarcoFalke merged this on Sep 5, 2018

MarcoFalke closed this on Sep 5, 2018

MarcoFalke referenced this in commit adf27b531a on Sep 5, 2018

laanwj commented at 10:22 am on October 16, 2018: member

so in case anyone cares, i’m not happy about this

“can we add this?” “well, "

I still strongly disagree with this, I don’t think we should pile up linters for cosmetic issues. The original idea of them was to flag serious issues that can result in bugs. Failing the tests on some supposed spelling error in a comment really, goes too far.

“but… can we add it if it only warns?”

Yes, warning is ok with me

Though I think it would be best if one of us runs this periodically, for example for every major release, and fixes the jarring typos. I don’t think this is something that necessarily needs to run continuously.

now, a few months later, it is causing Travis failures—apparently through #14179, without any discussion this is not nice…

practicalswift commented at 11:01 am on October 16, 2018: contributor

@laanwj Oh, I wasn’t aware of the policy change introduced in #14179 so I don’t have anything meaningful to add about that. Setting the discussion about that merge decision aside:

Do you have a link to the Travis failure? It would be interesting to see if it was a true positive or a false positive that caused codespell to complain.

luke-jr commented at 11:08 am on October 16, 2018: member

It came up when someone tried to name a mutex mut. Seems unreasonable to demand variables have English word names…

laanwj commented at 11:22 am on October 16, 2018: member

yes it came up on IRC @karel-3d

 008:29 < karelb> I am trying to run my fork of bitcoind on travis.... and travis stops with this
 108:29 < karelb> src/threadinterrupt.cpp:25: mut  ==> must, mutt, moot
 208:29 < karelb> Warning: codespell identified likely spelling errors
 308:29 < karelb> failure generated from test/lint/lint-spelling.sh
 408:29 < karelb> .....ummmmm, ok?
 508:30 < karelb> I did no change in threadinterrupt, and that "mut" is just some mutex
 608:39 < karelb> well whatever, one commit that renamed "mut" to "mutex" fixed that, I just wonder why I needed to do that...
 708:40 < gwillen> karelb: your travis instance is stupid
 808:40 < gwillen> if you make it less stupid you will have fewer problems
 908:40 < gwillen> it is trying to spell-correct your code, and doing it moronically
1008:40 < karelb> :D
1108:40 < gwillen> and seems to be set to warnings-as-errors.
1208:40 < karelb> I use travis-ci.org
1308:41 < gwillen> well, apparently they are stupid
1408:41 < gwillen> but I imagine there is an option not to make codespell warnings errors
1508:41 < karelb> the same config as bitcoin core
1608:41 < gwillen> which they absolutely should not be
1708:41 < gwillen> oh, hm
1808:41 < gwillen> I mean, seemingly not
1908:42 < karelb> well let's see what happens when I make a PR, if that `mut` stuff is still there
2010:49 < wumpus> karelb: lol that's stupid
2110:50 < wumpus> please don't tell me that stupid spelling check is mandatory in travis now

practicalswift commented at 11:27 am on October 16, 2018: contributor

Oh, that sounds really strange.

I’m unable to reproduce locally:

0$ grep mut src/threadinterrupt.cpp
1        LOCK(mut);
2    WAIT_LOCK(mut, lock);
3$ codespell src/threadinterrupt.cpp
4$ echo $?
50

Anyone who has been able to reproduce?

practicalswift commented at 11:30 am on October 16, 2018: contributor

I was able to reproduce after upgrading codespell. Will submit a PR fixing this.

scravy commented at 11:30 am on October 16, 2018: contributor

@practicalswift Which versions of codespell?

practicalswift commented at 11:35 am on October 16, 2018: contributor

@scravy

0$ pip3 install codespell --upgrade
1Requirement already up-to-date: codespell in /usr/local/lib/python3.6/dist-packages
2$ codespell --version
31.14.0

practicalswift commented at 11:40 am on October 16, 2018: contributor

Fixed in #14495 :-)

practicalswift commented at 11:59 am on October 16, 2018: contributor

@scravy Seems like this was introduced somewhere between 1.13.1 and 1.14.0. We’re pinning to 1.13.0 in Travis. I don’t know how a more recent got installed in @karel-3d:s Travis setup. @karel-3d, do you know? :-)