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

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

Merged
merged 2 commits into from Sep 5, 2018

Conversation

Projects
None yet
@practicalswift
Copy link
Member

commented Aug 13, 2018

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

@fanquake fanquake added the Tests label Aug 13, 2018

@practicalswift practicalswift force-pushed the practicalswift:lint-spell branch 2 times, most recently Aug 13, 2018

@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Travis failure:

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

@practicalswift practicalswift force-pushed the practicalswift:lint-spell branch 7 times, most recently Aug 13, 2018

.travis.yml Outdated
cache: false
language: python
python: '3.6'
install:
- travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq codespell

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Aug 13, 2018

Member

Can use

addons:
  apt:
    packages:
    - codespell

to avoid sudo

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 13, 2018

Author Member

Oh that is much cleaner! Thanks!

@practicalswift practicalswift force-pushed the practicalswift:lint-spell branch Aug 13, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

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.

test/lint/lint-spelling.sh Outdated

EXIT_CODE=0
EXCLUDE_REGEXP="(src/example.cpp:.*example-false-positive-word) "
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}")

This comment has been minimized.

Copy link
@Empact

Empact Aug 13, 2018

Member

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 13, 2018

Author Member

Good point! Now fixed.

src/rpc/net.cpp Outdated
"\n" "Immediately disconnects from the specified peer node.\n"
"\n" "Strictly one out of 'address' and 'nodeid' can be provided to identify the node.\n"
"\n" "To disconnect by nodeid, either set 'address' to the empty string, or call using the named 'nodeid' argument only.\n"
"\n" "Arguments:\n"

This comment has been minimized.

Copy link
@Empact

Empact Aug 13, 2018

Member

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 13, 2018

Author Member

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.

.travis.yml Outdated
addons:
apt:
packages:
- codespell

This comment has been minimized.

Copy link
@Empact

Empact Aug 13, 2018

Member

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 13, 2018

Author Member

Good point! Now fixed

@Empact

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Concept ACK

@practicalswift practicalswift force-pushed the practicalswift:lint-spell branch 4 times, most recently Aug 13, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

@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 :-)

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

@practicalswift practicalswift changed the title lint: Add spell check linter (codespell). Fix typos reported by codespell. lint: Check for common misspellings using codespell. Fix typos reported by codespell. Aug 13, 2018

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

This comment has been minimized.

Copy link
@MarcoFalke

This comment has been minimized.

Copy link
@Empact

Empact Aug 14, 2018

Member

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

src/qt/coincontroldialog.cpp Outdated
@@ -185,7 +185,7 @@ void CoinControlDialog::buttonSelectAllClicked()
ui->treeWidget->topLevelItem(i)->setCheckState(COLUMN_CHECKBOX, state);
ui->treeWidget->setEnabled(true);
if (state == Qt::Unchecked)
coinControl()->UnSelectAll(); // just to be sure
coinControl()->DeSelectAll(); // just to be sure

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 13, 2018

Member

Should probably be a scripted diff

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 14, 2018

Author Member

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

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.

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

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 practicalswift force-pushed the practicalswift:lint-spell branch Sep 4, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

@Sjors Squashed. Please re-review :-)

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

utACK b5ee29e460dc36066bac68e041073100c0eca20e

src/qt/coincontroldialog.cpp Outdated
@@ -441,7 +441,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
for (const auto& out : model->wallet().getCoins(vCoinControl)) {
if (out.depth_in_main_chain < 0) continue;

// unselect already spent, very unlikely scenario, this could happen
// deselect already spent, very unlikely scenario, this could happen

This comment has been minimized.

Copy link
@fanquake

fanquake Sep 4, 2018

Member

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 4, 2018

Author Member

Good point. Fixed!

@practicalswift practicalswift force-pushed the practicalswift:lint-spell branch to f8a81f7 Sep 4, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

utACK f8a81f7

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

This comment has been minimized.

Copy link
@Empact

Empact Sep 4, 2018

Member

I don't find hights in current master

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 5, 2018

Author Member

@Empact

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

:-)

@Empact

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

utACK ada3562 could squash

@MarcoFalke MarcoFalke merged commit f8a81f7 into bitcoin:master Sep 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 5, 2018

Merge #13954: Warn (don't fail!) on spelling errors. Fix typos report…
…ed by codespell.

f8a81f7 lint: Add spell check linter (codespell) (practicalswift)
ada3562 Fix typos reported by codespell (practicalswift)

Pull request description:

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

Tree-SHA512: 9974c0e640b411c7d0ebc5b45de253c19bac7fe3002cd98601ff8da8db584224c2fd7d331aee3df612c9f2cfef540d647a9b4c5a1a73fd208dc93ce4bf9e5e3e
@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

@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

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

yes it came up on IRC @karel-3d

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

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

Oh, that sounds really strange.

I'm unable to reproduce locally:

$ grep mut src/threadinterrupt.cpp
        LOCK(mut);
    WAIT_LOCK(mut, lock);
$ codespell src/threadinterrupt.cpp
$ echo $?
0

Anyone who has been able to reproduce?

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

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

@scravy

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

@practicalswift Which versions of codespell?

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

@scravy

$ pip3 install codespell --upgrade
Requirement already up-to-date: codespell in /usr/local/lib/python3.6/dist-packages
$ codespell --version
1.14.0
@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

Fixed in #14495 :-)

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

@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? :-)

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

I cloned the repo, did some experimental changes, and used Travis-ci.org.

The build is here

https://travis-ci.org/karel-3d/bitcoin/builds/442019337

It's branched from this commit, 18 days old. (Maybe it will go away if I rebase on current master?)

0809e68

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

Oh yeah. It seems I have branched the master before this commit

d10f2cd

Which means that if I rebase to master, it would indeed go away.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

@karel-3d Thanks for the clarification! That explains the mystery :-)

sipa added a commit that referenced this pull request Oct 17, 2018

Merge #14495: build: Warn (don't fail!) on spelling errors
c32cf6a Add ignored word: mut (practicalswift)
4ae50da Revert "qa: Fix codespell error and have lint-spelling error instead of warn" (practicalswift)

Pull request description:

  Revert `codespell` policy change introduced in #14179.

  Context: #13954 (comment)

Tree-SHA512: 4606b19bb32cdd661f90b3778759818d3493e5ed1a4a2f95982f07eeb6b9c889bc8d53cde31706e0a3b9524c3d3a7378f1b769a60baeb0d00da4c68fd3068114
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.