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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• test: p2p_feefilter improvements (logging, refactoring, speedup) #19564 (test: p2p_feefilter improvements (logging, refactoring, speedup) by theStack)
• RPC: Add ancestor{count,size,fees} to listunspent output #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

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: the idiomatic/Pythonic _ communicates clearly to the reader that the variable in question is a throwaway variable

While you're at it consider using vulture too to find more unused variables :)

for F in $(git ls-files -- "*.py"); do vulture "$F" | grep "unused variable"; done

Note that vulture might give you false positives, so make sure to check all candidates manually (probably goes without saying! :)).

Concept ACK. I'm really hesitant to review this without us trying to prevent it in the future.

Playing with vulture for a couple minutes seemed to give far too many false positives.

Pylint, on the other hand, surely had many false negatives, but seemed to do a good job otherwise.

pylint test/functional/**/*.py --disable all --enable W0612

Possible extension that I don't want to forget: replace range(len(foo)) with enumerate(foo).

pylint test/functional/**/*.py --disable all --enable W0612

I think pylint $(git ls-files -- "*.py") --disable all --enable W0612 will catch more cases :)

The reason we disabled vulture in Travis was due to false positives. Is pylint's W0612 check free from false positives in our code base?

(False negatives are not a big deal, but false positives are :))

The reason we disabled vulture in Travis was due to false positives. Is pylint's W0612 check free from false positives in our code base?

Just manually checked on this PR. Surprisingly, yes! All instances can be replaced, by either _ or del foo in the case of an unused parameter (2.1.4 of https://google.github.io/styleguide/pyguide.html).

-0 on this, I agree this looks somewhat neater, but also, I'm not sure it's worth making this change and being pedantic about it in the linter.

Thanks for the conceptual reviews and linting tipps so far!

Concept ACK. I'm really hesitant to review this without us trying to prevent it in the future.

Yes, I was thinking the same. My hope is that with the pattern being more widespread in the code, awareness for this raises and hence it is on the long term easier catched both by PR creators and by reviewers. Not sure if that's enough -- adding it as rule to the linter seems to be too strong though, agree with laanwj here.

For anyone wanting to try out the linters: note that (at least on my system) pylint is for linting Python 2 code, hence we should ensure to use pylint3 here, which interestingly enough also detects a bit more. (Some systems may have a symlink from pylint to pylint3, but better check via pylint --version.)

ACK dac7a11 -- the updated code is easier to reason about since the throwaway nature of a variable is expressed explicitly (using the Pythonic _ idiom) instead of implicitly. Explicit is better than implicit was we all know by now :)

Explicit is better than implicit was we all know by now :)

Yes, but also some people contributing to the test framework have been complaining about the Python linter framework that it does 'silly' checks that don't really help catch errors, but just feel overly pedantic. I think this is one such one.

Edit: To be clear:
ACK on this change as it is now, NACK on adding a linter for this

manual inspection ACK dac7a11

though I think this kind of thing is a very low risk of testing bugs.