You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/05/30 17:44:57 UTC
[GitHub] [incubator-superset] john-bodley opened a new issue #9953: [SIP-46] Proposal for strict pylint enforcement
john-bodley opened a new issue #9953:
URL: https://github.com/apache/incubator-superset/issues/9953
## [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`](https://en.wikipedia.org/wiki/Pylint), a powerful yet sometimes perceived unusable quality checker.
Though `pylint` is enabled as part of [CI](https://github.com/apache/incubator-superset/blob/b8eaa114eddc6a42e2b69b7ab88479c3128a52be/.github/workflows/superset-python.yml#L28) and [`tox`](https://github.com/apache/incubator-superset/blob/52285aeb04ccad1611f88afdec92080ae04bdc0e/tox.ini#L142) many of the checks are ignored either [globally](https://github.com/apache/incubator-superset/blob/980dd2fd419893ec5ed9386ca28fa87115a3753d/.pylintrc#L84) 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](https://github.com/apache/incubator-superset/pulls?q=is%3Apr+%5Bflake8%5D+Resolving)) and `mypy` ([PRs](https://github.com/apache/incubator-superset/pulls?q=is%3Apr+style%28mypy%29+)). 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 checkers disabled) and unequivocally say that having `pylint` enabled as help to improve the code quality (remedying a number of bugs), 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).
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](https://pypi.org/project/pylint/2.5.2/) at time of writing). Currently we are using `pylint` [1.9.2](https://pypi.org/project/pylint/1.9.2/) which was released on June 6, 2018.
2. Systematically† remove the globally [disabled](https://github.com/apache/incubator-superset/blob/980dd2fd419893ec5ed9386ca28fa87115a3753d/.pylintrc#L84) 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 checker (or small subset of checkers) and/or file(s) at a time.
‡ Note based on other repos which use `pylint` all checkers could be disabled except for:
- `bad-continuation` which is incompatible with `black`.
- `missing-module-docstring` since Python modules are typically not documented at the module/file level.
### New or Changed Public Interfaces
N/A
### New dependencies
N/A
### Migration Plan and Compatibility
N/A
### Rejected Alternatives
N/A
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on issue #9953: [SIP-46] Proposal for strict pylint enforcement
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #9953:
URL: https://github.com/apache/incubator-superset/issues/9953#issuecomment-645792908
Tried to bump `pylint` here https://github.com/apache/incubator-superset/pull/10095 just to see if that'll work now that they release new versions and that we're using Github Actions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on issue #9953: [SIP-46] Proposal for strict pylint enforcement
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #9953:
URL: https://github.com/apache/incubator-superset/issues/9953#issuecomment-645130658
+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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9953: [SIP-46] Proposal for strict pylint enforcement
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9953:
URL: https://github.com/apache/incubator-superset/issues/9953#issuecomment-646099952
Bumped pylint and fixed most of the new checks here: https://github.com/apache/incubator-superset/pull/10101
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org