[SIP-46] Proposal for strict Pylint enforcement

[john-bodley]

[SIP] Proposal for strict pylint enforcement

Motivation

Over the past number of years there have been a number of initiatives to improve the quality of the Python code via a number of linters (flake8; deprecated), formatters (black, isort), and type checkers (mypy). The one elephant in the room is pylint, a powerful yet sometimes perceived unusable quality checker.

Though pylint is enabled as part of CI and tox many of the checks are ignored either globally or at the file level via:

# pylint: disable=C,R,W

From previous experiences the value of many of these linters/checkers only materializes when strict enforcement exists for the entire repo (somewhat akin to heard immunity). Hence rather than merely trying to ensure that new code meets a higher bar, we (as a community) need to roll up our sleeves and take the somewhat painstaking approach of systematically re-enabling checks, removing blanket disablement, etc. somewhat akin to how we were able to strictly enforce flake8 (PRs) and mypy (PRs). Once the entire code base adheres to the strictly enforced rules it is then relatively simple (in terms of effort) to ensure a PR adheres to the necessary checks.

Proposed Change

I propose that by strictly enforcing pylint we will drastically improve the quality, readability, and maintainability of the Python code. I have worked on a number of repos which strictly enforce pylint (with very few checks disabled) and unequivocally can say that having pylint enabled helped to improve the code quality (preventing a number of bugs), consistency, readability, etc. Additionally I have not found pylint to be unusable as others have stated and do not find it cumbersome to use (once the necessary ground work has been laid).

Note the scale and effort involved is greater than any of the previous checkers and thus will require a systematic approach (hopefully undertaken by a number of individuals in the community). This initiative may take a number of months to complete, i.e., for context having the Python code fully adhere to having all function definitions be typed took around three months with limited involvement.

The three steps which need to be undertaken (in order) are:

1. Bumping the version of pylint to the latest stable version (2.5.2 at time of writing). Currently we are using pylint 1.9.2 which was released on June 6, 2018.
2. Systematically† remove the globally disabled checkers‡.
3. Systematically† reenable checking for all files, i.e., removing the # pylint: disable=C,R,W comments.

† Note systematically implies via a slew of PRs which re-enables a check (or small subset of checks) and/or file(s) at a time.

‡ Note based on other repos which use pylint the only the following checks should be disabled:

• bad-continuation which is incompatible with black. This was removed in Pylint 2.6 as it was deemed black or other formatters are better equipped to handle this.
• missing-module-docstring since Python modules are typically not documented at the module/file level. This isn’t necessary given all files contain a copyright header.

Additionally the following checks should be enabled:

• useless-suppression ensures there are no unnecessary # pylint: disable suppressions.

New or Changed Public Interfaces

N/A

New dependencies

N/A

Migration Plan and Compatibility

N/A

Rejected Alternatives

A list of Python static analysis tools are described here. Note that we previously used flake8 (in conjunction with pylint) but later deprecated it as felt that black, isort, and pylint represented a superset in terms of functionality.

[mistercrunch]

+1, I'm supportive of the effort, happy to take one some of the work and I would love to involve the wider community for this effort. Maybe we can make a call to action on Slack / the mailing list and use this as on ramp for more contributions.

Side note I remember I tried to bump pylint to 2.x at least twice in the past while we were on travis and it was slower than the current version, and would go 10+ minutes without std output (there was no verbose option that would output progress) and Travis would die / timeout. Hoping most of this is resolved.

[mistercrunch]

Tried to bump pylint here #10095 just to see if that'll work now that they release new versions and that we're using Github Actions

[willbarrett]

Bumped pylint and fixed most of the new checks here: #10101

[rusackas]

I don't believe this has gone up for a DISCUSS thread on the dev@ list, has it @john-bodley ? Shall we push this through?

[cclauss]

I concur with the sentiment stated above that pylint requires so many linter directives that it is sometimes perceived as unusable. Black and isort have their place but are not really focused on runtime safety and program correctness.

This repo currently has two undefined names in Python code that none of the tools above would find. These appeared without being detected since #3264

I would encourage the use of the following command as part of the linting arsenal…

$ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics

https://flake8.pycqa.org/en/latest/user/error-codes.html

This flake8 test selection does not focus on "style violations" (the majority of flake8 error codes that psf/black can autocorrect). Instead, these tests focus on runtime safety and correctness:

• E9 tests are about Python syntax errors usually raised because flake8 can not build an Abstract Syntax Tree (AST). Often these issues are a sign of unused code or code that has not been ported to Python 3. These would be compile-time errors in a compiled language but in a dynamic language like Python, they result in the script halting/crashing on the user.
• F63 tests are usually about the confusion between identity and equality in Python. Use ==/!= to compare str, bytes, and int literals is the classic case. These are areas where a == b is True but a is b is False (or vice versa). Python >= 3.8 will raise SyntaxWarnings on these instances.
• F7 tests logic errors and syntax errors in type hints
• F82 tests are almost always undefined names which are usually a sign of a typo, missing imports, or code that has not been ported to Python 3. These also would be compile-time errors in a compiled language but in Python, a NameError may be raised which will halt/crash the script on the user.

[john-bodley]

The vote has officially passed.