lint: convert spellchecking lint test to python

fjahr commented at 9:49 pm on April 4, 2022: member

The new python version should produce the exact same output as the bash version but be easier to maintain.

fjahr force-pushed on Apr 4, 2022

DrahtBot added the label Tests on Apr 4, 2022

DrahtBot commented at 11:32 pm on April 4, 2022: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24750 (doc: Add bash dependency of lint tests by fjahr)
#24435 (test: Refactor subtree exclusion in lint tests by maxraustin)

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.

fanquake commented at 8:00 am on April 5, 2022: member

Concept ACK - can you rebase now that #24762 has been merged.

in test/lint/lint-spelling.py:19 in 494e9918ee outdated

14+ignore_words_file = 'test/lint/spelling.ignore-words.txt'
15+files_args = r'git ls-files -- ":(exclude)build-aux/m4/" ":(exclude)contrib/seeds/*.txt" ":(exclude)depends/" ":(exclude)doc/release-notes/" ":(exclude)src/leveldb/" ":(exclude)src/crc32c/" ":(exclude)src/qt/locale/" ":(exclude)src/qt/*.qrc" ":(exclude)src/secp256k1/" ":(exclude)src/minisketch/" ":(exclude)src/univalue/" ":(exclude)contrib/builder-keys/keys.txt" ":(exclude)contrib/guix/patches"'
16+
17+
18+def main():
19+    files = check_output(files_args, shell=True).decode("utf-8").splitlines()

laanwj commented at 9:44 am on April 5, 2022:

Please don’t use shell=true if you can avoid it. Remember that a point of doing this porting is to avoid shell ambiguities.

fjahr commented at 10:20 pm on April 5, 2022:

was able to avoid it, done

laanwj commented at 9:45 am on April 5, 2022: member

Concept ACK, thanks!

theStack commented at 5:41 pm on April 5, 2022: member

Concept ACK

brunoerg commented at 5:49 pm on April 5, 2022: member

Concept ACK

fjahr force-pushed on Apr 5, 2022

lint: convert spell check lint test to python 77f98df41f

doc: Update lint test docs 4685463301

fjahr force-pushed on Apr 5, 2022

fjahr commented at 10:22 pm on April 5, 2022: member

Rebased and added the error handling in case codespell is not installed which I had omitted in the last push and also avoided usage of shell=True.

MarcoFalke commented at 6:05 am on April 6, 2022: member

cr ACK 4685463301a1c64c1be07725059cc94d69db104b

MarcoFalke approved

in test/lint/lint-spelling.py:33 in 4685463301

28+
29+    files = check_output(FILES_ARGS).decode("utf-8").splitlines()
30+    codespell_args = ['codespell', '--check-filenames', '--disable-colors', '--quiet-level=7', '--ignore-words={}'.format(IGNORE_WORDS_FILE)] + files
31+
32+    try:
33+        check_output(codespell_args, stderr=STDOUT)

MarcoFalke commented at 6:11 am on April 6, 2022:

Will this print the spelling mistakes?

fjahr commented at 6:45 am on April 6, 2022:

For me it does but testing on more platforms might be a good idea :)

0$ test/lint/lint-spelling.py
1configure.ac:364: overriden ==> overridden
2configure.ac:860: overriden ==> overridden
3src/wallet/coinselection.h:228: Chooose ==> Choose
4^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt

MarcoFalke commented at 7:12 am on April 6, 2022:

Yeah, it should. I just missed the print(e.output.decode line below. :man_facepalming:

MarcoFalke commented at 7:21 am on April 6, 2022:

nit: I guess an alternative would be to use subprocess.check_call and not redirect the output at all or print() it?

MarcoFalke merged this on Apr 6, 2022

MarcoFalke closed this on Apr 6, 2022

in test/lint/lint-spelling.py:12 in 4685463301

 7+"""
 8+Warn in case of spelling errors.
 9+Note: Will exit successfully regardless of spelling errors.
10+"""
11+
12+from subprocess import check_output, STDOUT, CalledProcessError

MarcoFalke commented at 7:19 am on April 6, 2022:

nit for future rewrites. I slightly prefer import subprocess and then subprocess.symbol in the code below. It is more verbose but clearer.

in test/lint/lint-spelling.py:15 in 4685463301

10+"""
11+
12+from subprocess import check_output, STDOUT, CalledProcessError
13+
14+IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt'
15+FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/leveldb/", ":(exclude)src/crc32c/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)src/univalue/", ":(exclude)contrib/builder-keys/keys.txt", ":(exclude)contrib/guix/patches"]

MarcoFalke commented at 7:20 am on April 6, 2022:

nit: Add trailing , and put each token on a newline to make editing and reading easier?

in test/lint/lint-spelling.py:36 in 4685463301

31+
32+    try:
33+        check_output(codespell_args, stderr=STDOUT)
34+    except CalledProcessError as e:
35+        print(e.output.decode("utf-8"), end="")
36+        print('^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in {}'.format(IGNORE_WORDS_FILE))

MarcoFalke commented at 7:22 am on April 6, 2022:

nit: Can use f-strings in new code

MarcoFalke commented at 7:22 am on April 6, 2022: member

Left some nits for your future transpiles, feel free to ignore.

sidhujag referenced this in commit ed33fb1e90 on Apr 6, 2022

jonatack commented at 7:19 am on April 7, 2022: member

ACK, working for me on debian.

laanwj referenced this in commit 57a73d71a3 on Apr 18, 2022

DrahtBot locked this on Apr 7, 2023

lint: convert spellchecking lint test to python #24766

Conflicts