-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Add pylint in CI "lint" task #4308
Comments
Do you want really add Line 68 in f076ca5
|
In this issue I am proposing adding
|
Oh, OK. |
Hi. How about using pre-commit instead? For example, it is already set up in Jupyterhub and Airflow repositories: |
Thanks very much for your suggestion and interest in LightGBM. I don't think it is correct to say that You are welcome to open a separate issue proposing the use of |
Summary
I think that
pylint
could be used in this project to provide higher confidence that certain types of bugs in Python code will fail builds and not make it into releases.Motivation
For example, in #4143 I accidentally introduced a typo in a variable reference used in an f-string. This wasn't caught by review or CI, and happened on a line that wasn't covered by unit tests.
As I mentioned in https://github.com/microsoft/LightGBM/pull/4293/files#r636513196, this particular type of error would have been caught by
pylint
.Other classes of error that can be caught by
pylint
:all([x for x in things])
instead ofall(x for x in things)
full list of possible warnings, from "pylint --list-msgs" as of pylint 2.7.0 (click me)
I've also found that
pylint
is really helpful for at least providing some coverage of code branches that are difficult or expensive to write unit tests for.Description
I'd like to propose that we add
pylint
in LightGBM's CI. I think that it should only be used to catch bugs in correctness or efficiency, and that all style-only warnings should be suppressed.I'd like to do the following, similar to the approaches taken for
cpplint
(#1990) andmypy
(#3867):pylint
to the "lint" job in CI, but make sure that it always passes. Likepylint python-package/ || true
pylint
a required check in CI (e.g.pylint python-package/ || exit -1
)References
pylint
documentation: https://docs.pylint.org/en/1.6.0/index.htmlThe text was updated successfully, but these errors were encountered: