lint: convert spellchecking lint test to python #24766

pull fjahr wants to merge 2 commits into bitcoin:master from fjahr:pr24762-spelling changing 3 files +42 −23

fjahr commented at 9:49 PM on April 4, 2022: contributor

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
fjahr cross-referenced this on Apr 4, 2022 from issue lint: Start to use py lint scripts by MarcoFalke
DrahtBot commented at 11:32 PM on April 4, 2022: contributor


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.
DrahtBot cross-referenced this on Apr 5, 2022 from issue doc: Add bash dependency of lint tests by fjahr
fanquake commented at 8:00 AM on April 5, 2022: member

Concept ACK - can you rebase now that #24762 has been merged.
DrahtBot cross-referenced this on Apr 5, 2022 from issue test: Refactor subtree exclusion in lint tests by maxraustin

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: contributor

Concept ACK
brunoerg commented at 5:49 PM on April 5, 2022: contributor

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: contributor

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

$ test/lint/lint-spelling.py
configure.ac:364: overriden ==> overridden
configure.ac:860: overriden ==> overridden
src/wallet/coinselection.h:228: Chooose ==> Choose
^ 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.
laanwj cross-referenced this on Apr 6, 2022 from issue Port lint scripts to Python by laanwj
sidhujag referenced this in commit ed33fb1e90 on Apr 6, 2022
fjahr cross-referenced this on Apr 6, 2022 from issue lint: Convert Python linter to Python by fjahr
jonatack commented at 7:19 AM on April 7, 2022: contributor

ACK, working for me on debian.
laanwj referenced this in commit 57a73d71a3 on Apr 18, 2022
aureleoules cross-referenced this on Jul 26, 2022 from issue lint: Make lint spellcheck a failure instead of warning by aureleoules
bitcoin locked this on Apr 7, 2023

Contributors

Labels

Linked (view graph)

#24762 lint: Start to use py lint scripts #24783 Port lint scripts to Python #24794 lint: Convert Python linter to Python #25706 lint: Make lint spellcheck a failure instead of warning

lint: convert spellchecking lint test to python #24766

Conflicts