lint: convert spellchecking lint test to python #24766
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK - can you rebase now that #24762 has been merged. |
|
Concept ACK, thanks! |
|
Concept ACK |
|
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 |
|
cr ACK 4685463 |
| codespell_args = ['codespell', '--check-filenames', '--disable-colors', '--quiet-level=7', '--ignore-words={}'.format(IGNORE_WORDS_FILE)] + files | ||
|
|
||
| try: | ||
| check_output(codespell_args, stderr=STDOUT) |
There was a problem hiding this comment.
Will this print the spelling mistakes?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, it should. I just missed the print(e.output.decode line below. 🤦♂️
There was a problem hiding this comment.
nit: I guess an alternative would be to use subprocess.check_call and not redirect the output at all or print() it?
| Note: Will exit successfully regardless of spelling errors. | ||
| """ | ||
|
|
||
| from subprocess import check_output, STDOUT, CalledProcessError |
There was a problem hiding this comment.
nit for future rewrites. I slightly prefer import subprocess and then subprocess.symbol in the code below. It is more verbose but clearer.
| from subprocess import check_output, STDOUT, CalledProcessError | ||
|
|
||
| IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt' | ||
| 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"] |
There was a problem hiding this comment.
nit: Add trailing , and put each token on a newline to make editing and reading easier?
| codespell_args = ['codespell', '--check-filenames', '--disable-colors', '--quiet-level=7', '--ignore-words={}'.format(IGNORE_WORDS_FILE)] + files | ||
|
|
||
| try: | ||
| check_output(codespell_args, stderr=STDOUT) |
There was a problem hiding this comment.
nit: I guess an alternative would be to use subprocess.check_call and not redirect the output at all or print() it?
| check_output(codespell_args, stderr=STDOUT) | ||
| except CalledProcessError as e: | ||
| print(e.output.decode("utf-8"), end="") | ||
| print('^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in {}'.format(IGNORE_WORDS_FILE)) |
There was a problem hiding this comment.
nit: Can use f-strings in new code
47b66ac lint: Convert Python linter to Python (Fabian Jahr) Pull request description: The outputs provided by the Python version should be exactly the same as the ones from the shell version. There is small improvement here: Previously only the dependency of `flake9` was checked, now all dependencies are checked before running. I also tried to mostly follow the [recommendations here](bitcoin/bitcoin#24766 (review)) but happy to make more changes if there is still room for improvement. ACKs for top commit: laanwj: Tested ACK 47b66ac Tree-SHA512: 1630188e176c1063b8905669b76682b361a858cde6990ab17e51ad4333bf376eab796050cdb9f2967b84f1f74379d9e860c4258561b1964e1a47183c593e5bb4
The new python version should produce the exact same output as the bash version but be easier to maintain.