Turn on a lot more pylint

[dcbaker]

With recent flake8 cleanups, I thought it might be a good time to turn on more pylint checks. In the process I've flipped pylint from an allow-list to a deny-list of checks. This should help us not introduce new issues, as well as make it easier to clean up existing ones. There's a lot of code churn here, but it's all refactoring with no real behavioral changes intended. This does leave quite a bit more to do with pylint, but I felt this was enough as-is and didn't need more added on

[lgtm-com[bot]]

This pull request introduces 1 alert and fixes 1 when merging eae0aaa839524645c89c955bb2586be47d9ea9bc into 251113fa67c90480707cae9341dd8387f3dd6bbd - view on LGTM.com

new alerts:

• 1 for Unused local variable

fixed alerts:

• 1 for Unreachable code

[eli-schwartz]

I actually don't like the use of pyproject.toml for so many things.

dotfiles are an ethically sound approach to configuration, they don't introduce any cognitive overhead on looking at the source tree and don't affect anything that doesn't specifically go looking for them. They don't even introduce a parse-time overhead for looking at small portions of a file that you'll never use. ;)

More importantly, flake8 refuses to implement pyproject.toml support (and I think their reasoning is sound for doing so). Other parts of the ecosystem have "support" for pyproject.toml that is inferior, and significantly so, to using the standalone config file -- see e.g. tox's support for a tox section in pyproject.toml which contains a string assignment for an inlined configparser-formatted config file, which is read via toml, accessed as a string, then passed to configparser as a file buffer. All to avoid one file that no one has to look at!

It's not like we're actually getting rid of all those dotfiles that don't bother anyone, anyway. We have 8 of them, pylintrc is only one of them, and two of them are git-specific, .editorconfig isn't a python tool and doesn't buy into "pyproject" or "toml", and .lgtm.yml is a yaml configuration for a python checker. azure-pipelines.yml isn't even a dotfile, which is something I find a lot more bothersome. We also have .github/ which has tons of files in it, logically grouped together... I guess it would be nice if there was some sort of standard for storing developer tool configuration files in idk, .devtools/*{rc,.{ini,toml}}

On a personal level, I find it a big pain to read pyproject.toml in search of one tool's configuration and see the config for lots of other tools, then have to scroll a lot in order to find what I am actually interested in. This currently isn't a problem because we only use that file to manually specify the default pip behavior for build-system.requires (and later to specify which one of two almost-interchangeable setuptools backends to use).

But it will be a problem once we store the [project] metadata there, which we are going to do once Meson has an in-tree backend, and suddenly have pylintrc and Meson itself fighting over UX space.

[dcbaker]

I've droped the pyproject.toml changes. I don't agree, but I don't want to argue it here, we'll do that another time, in another PR :)

I've reordered the commits so the switch to a deny-list is now the first commit, and each subsequent commit enables one (or in one case two closely related) checks in each commit. I know there's a small bug in the cuda module still due to a missing re-indent, and the pylint in our image is apparently too old to understand some mypy conventions. I'm happy to break this into smaller chunks if that would help with review

[lgtm-com[bot]]

This pull request introduces 1 alert and fixes 1 when merging 531ab846062154cf3c362dce488c869d3f8bed1c into 7bdfe7ccb224d65b21feea4c6a92ddf57ca0c18c - view on LGTM.com

new alerts:

• 1 for Unused local variable

fixed alerts:

• 1 for Unreachable code

[codecov[bot]]

Codecov Report

Merging #10767 (536c779) into master (0a6e485) will increase coverage by 0.00%. The diff coverage is 79.02%.

@@           Coverage Diff           @@
##           master   #10767   +/-   ##
=======================================
  Coverage   68.93%   68.94%           
=======================================
  Files         414      414           
  Lines       90526    90570   +44     
  Branches    20735    20699   -36     
=======================================
+ Hits        62407    62446   +39     
- Misses      23410    23419    +9     
+ Partials     4709     4705    -4     

Impacted Files	Coverage Δ
mesonbuild/_typing.py	0.00% <ø> (ø)
mesonbuild/backend/vs2010backend.py	0.00% <0.00%> (ø)
mesonbuild/backend/vs2013backend.py	0.00% <0.00%> (ø)
mesonbuild/backend/xcodebackend.py	0.00% <0.00%> (ø)
mesonbuild/compilers/d.py	50.73% <ø> (ø)
mesonbuild/dependencies/cuda.py	19.23% <0.00%> (ø)
mesonbuild/interpreter/kwargs.py	0.00% <0.00%> (ø)
mesonbuild/mdist.py	75.00% <0.00%> (ø)
mesonbuild/modules/dlang.py	25.28% <0.00%> (ø)
mesonbuild/scripts/delwithsuffix.py	0.00% <0.00%> (ø)
... and 219 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[dcbaker]

I've droped the no-else-* commits (they're huge and this series is big enough), but added a couple more smaller, simpler, and more obviously problematic if found checks, plus fixed the checks that fail in CI but pass on my system due to an older pylint

[tristan957]

Did you have more PRs you wanted to split off of this one?

[dcbaker]

@eli-schwartz: now rebased on master

[eli-schwartz]

A couple things which I think are rebase errors.

• pylint: enable global-variable-not-assigned -- This was merged in #10834, commit now has no content
• pylint: enable unnecessary-comprehension -- This was merged in #10889, commit now has no content
• pylint: enable use-implicit-booleaness-not-comparison -- This re-enables use-a-generator, that got merged in #10889
  • pylint: enable use-a-generator -- empty except for reverting the previous mistake
• pylint: enable unnecessary-lambda-assignment -- this got dropped, and then pylint: enable cell-var-from-loop included some of its changes

[dcbaker]

Yup, there were a couple of empty commits (or almost empty commits). There cell-var-from-loop thing is catching that those functions are defined inside the loop, so I left them in that commit to get smaller more exact diffs

[eli-schwartz]

mesonmain: fix checking for deprecation notice

This still seems wrong to me, and furthermore the set_membership commit doesn't work on its own because it relies on the mesonmain commit removing a use of tuple membership.

Again, why do we need this mesonmain commit? The problem it claims to fix is verified to not be a problem, and it claims an object is a list when debug printfs reveal it is a string. Go ahead and add assert isinstance(options.command, list) if you're positive that it's a list...

(Sometimes I really hate being able to treat strings as iterable. This is one of those times.)

[dcbaker]

straings as iterables is useful, but I really wish that they didn't implement __iter__ and instead had an .iter() method so you have to be explicit that you wanted to iterate the string. I'll look at the mesonmain commit again. I know that I found a case where we weren't warning but we should be, but now I can't figure out what that case was (I should have written a better commit message) I guess I'll just drop the patch until we find that case again.

[eli-schwartz]

Actually it turns out that due to another bug, options.command was sometimes not actually the argparse command, and was in fact a list. I submitted a PR which fixes that.

[tristan957]

https://github.com/mesonbuild/meson/actions/runs/4079331616/jobs/7030588619#step:5:13

[dcbaker]

Yup. It’s a false positive due to the side effect. Also only shows up with plyint 2.16, I need to file a pylint bug