Flake8 For Python Tests #12884

The contributing guide asks for users to use flake8 to check their Python code prior to submitting. That said, a simple:

flake8 * | wc -l

In the test directory on master yield 7,731 warnings.

If there's an appetite for it I'd be happy to start cleaning some of these up. It's way too much for one PR but I could put a running checklist here and knock out module by module.

Thinking beyond this change, if we get all of the internal items cleaned up then flake8 could be added to CI to automatically detect and error out whenever coding guidelines have been violated. Thoughts?

Many of flake8 warnings are just about whitespace, which we don't care too much about. Those can be cleaned up when the code is touched for other reasons.

Though, we already enabled warnings on travis that could lead to bugs. See ./contrib/devtools/lint-python.sh.

I don't think there is anything left to do here. Closing for now.

I'm generally pro-flake8 linting. I don't think it's worth opening PRs specifically to clean up flake8 warnings, since that would lead to a lot of rebasing for other PRs. I think the following approach is better:

• fix flake8 warnings in modules when touching that module (as the first commit in a PR)
• when all (or almost all) the instances of a flake8 warning have been resolved, consider adding that warning to https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/lint-python.sh

Thanks for the insights and understood on the rebasing challenge. My only concern with excluding so many checks in the overall linting process is that it the exceptions can easily start to become the norm.

Taking E501 as an example there are close to 3,000 instances of lines that are too long per PEP8. Just eyeballing this a lot of them are only a few characters beyond the 79 char standard limit and I'd question for readability purposes if we really should allow those exceptions.

Because I haven't seen it in the code, you are aware that flake8 accepts inline comments to make exceptions right? So instead of ignoring at the overall CI level you could do something like:

...some_really_long_url_on_one_line  # noqa: E501

and the linter will ignore that particular line, while still catching others.

I personally don't think E501 is important, since I don't think a too-long-line is going to hide genuine bugs. I'd certainly be NACKish on PRs to 'fix' too-long-lines.

exceptions can easily start to become the norm.

Note that we didn't used to do any linting at all, so at least we're going in the right direction!

you are aware that flake8 accepts inline comments to make exceptions right?

Yes, but I personally don't like littering code with linter hints. I think it distracts readers from what the code is actually trying to communicate.