Update pylint to version 3.0#1310
Conversation
* Remove leading `_` from type alias (since it is type annotations it is effectively a public name) * Disable the cyclic import checker. By its definition, we do have cyclic imports. pylint seems not to find them when using multiple processes but it does find them when using a single process.
|
I want a newer version of pylint for #1285 where I want to use it to replace some of the benefits we were getting from |
coruscating
left a comment
coruscating
left a comment
There was a problem hiding this comment.
LGTM overall, just a minor question.
| no-else-return, # relax "elif" after a clause with a return | ||
| docstring-first-line-empty, # relax docstring style | ||
| import-outside-toplevel, | ||
| cyclic-import, # This checker raises on all module pairs that import each other, |
There was a problem hiding this comment.
Should we also turn off parallelism when running pylint to catch more errors potentially now that this check has been disabled?
There was a problem hiding this comment.
I am not sure what else there is that can get caught with parallelism disabled. Our tests are slower so there is not much downside to removing parallelism for the CI runs (I see around 10 mins for test and under 3 for pylint with parallelism). Maybe it should be left on for a run someone would call locally, or at least easy to opt into. Or maybe people don't generally run it on all files locally any way (versus just on the file being edited). What do you think?
There was a problem hiding this comment.
I added a commit to remove the parallelism but make it possible to enable it optionally with tox by moving the posargs location. I don't think it's common to want extra arguments to black?
There was a problem hiding this comment.
Thanks, that looks good! I haven't heard of people passing arguments to black. And I don't know specifically what removing parallelism might catch, but it seems fine since linting isn't that slow.
There was a problem hiding this comment.
I actually ran into a reason to pass an argument to black. tox runs with --check without --diff. You might want to run with --diff to see what the changes would be. It's simple enough just to run -eblack --diff in that case though.
There was a problem hiding this comment.
By the way, the time only changed from 2:46 to 2:54 for the two runs here with and without parallelism. There could be more difference locally on a system with more cores. The GitHub runners seem to have only two.
Move `posargs` from black to pylint for the tox lint commands so that pylint parallelism can be opted into.
|
By the way, I ended up not needing this for #1285. I was going to replace some of the checking that |
* Remove leading `_` from type alias (since it is type annotations it is effectively a public name) * Disable the cyclic import checker. By its definition, we do have cyclic imports. pylint seems not to find them when using multiple processes but it does find them when using a single process. * Remove default parallelism from tox pylint runs and from the CI run * Move `posargs` from black to pylint in the tox lint commands so that parallelism can be used optionally
2 participants
_from type alias (since it is type annotations it is effectively a public name)posargsfrom black to pylint in the tox lint commands so that parallelism can be used optionally