It does no control flow analysis, and upgrading to pylint 3.2.0 produced
many incorrect warnings.
Also ignore contextmanager-generator-missing-cleanup for now. It has FP
when there is no code after a yield (and thus no cleanup needs to be
handled), which is what we do. Currently under discussion upstream:
https://github.com/pylint-dev/pylint/issues/9625
Which adds the `use-set-for-membership` check. It's generally faster in
python to use a set with the `in` keyword, because it's a hash check
instead of a linear walk, this is especially true with strings, where
it's actually O(n^2), one loop over the container, and an inner loop of
the strings (as string comparison works by checking that `a[n] == b[n]`,
in a loop).
Also, I'm tired of complaining about this in reviews, let the tools do
it for me :)
This finds uses of deny-listed functions, which defaults to map and
filter. These functions should be replaced by comprehensions in
idiomatic python because:
1. comprehensions are more heavily optimized and are often faster
2. They avoid the need for lambdas in some cases, which make them
faster
3. you can do the equivalent in one statement rather than two, which
is faster
4. They're easier to read
5. if you need a concrete instance (ie, a list) then you don't have
to convert the iterator to a list afterwards
The one case of this was a false-positive, but what we were doing
(checking locals()) is not idiomatic. I've replaced the call to
`locals()` with the obvious `var: T.Optional[str] = None` with check
instead.
Which catches a very real bug. The zlib system dependency failed to work
on MSVC since initial implementation in commit
c1a3b37ab7 -- it looked for the wrong
name.
This catches some optimization problems, mostly in the use of `all()`
and `any()`. Basically writing `any([x == 5 for x in f])` vs `any(x == 5
for x in f)` reduces the performance because the entire concrete list
must first be created, then iterated over, while in the second f is
iterated and checked element by element.
This does force a number of uses of `# pylint: disable` comments, but it
also finds a couple of useless global uses and one place (in the
previous commit) that an easy refactor removes the use of global. Global
is a code smell, so forcing adding a comment to disable helps force
developers to really consider if what they're doing is a good idea.
The `global` statement is only needed to assign to global variables, not
read or mutate them. So calling `global.mutate()` is fine, but not
`var = foo`, which would otherwise shadow `var`.
This makes it much easier to see what we're ignoring, as well as
allowing pylint to enforce any lints that currently pass but aren't in
the allow list automatically.
I ran into one of these from LGTM, and it would be nice if pylint could
warn me as part of my local development process instead of waiting for
the CI to tell me.
This caught a couple of cases of us doing:
```python
for i in range(len(x)):
v = x[i]
```
which are places to use enumerate instead.
It also caught a couple of cases of:
```python
assert len(x) == len(y)
for i in range(len(x)):
xv = x[i]
yv = y[i]
```
Which should instead be using zip()
```python
for xv, yv in zip(x, y):
...
```
This didn't actually catch what it's supposed to, which is cases of:
```python
for x in dict.keys():
y = dict[x]
```
But it did catch one unnecessary use of keys(), and one case where we
were doing something in an inefficient way. I've rewritten:
```python
if name.value in [x.value for x in self.kwargs.keys() if isinstance(x, IdNode)]:
```
as
``python
if any((isinstance(x, IdNode) and name.value == x.value) for x in self.kwargs):
```
Which avoids doing two iterations, one to build the list, and a
second to do a search for name.value in said list, which does a single
short circuiting walk, as any returns as soon as one check returns True.
The last instances of
try:
...
except:
...
were removed in bf98ffca. The sideci.yml file was updated, but the
flake8 config still allows this. Ensure that flake8 tests fail if this
questionable construct appears again.
We're using these now, so having some error checking to make sure we
don't have paths were we're trying to instantiate an abstract class
would be good.
This catches some very real errors.
The one in scalapack is pretty silly actually, it's failing to figure
out that the exploded list is at least two arguments. However, the code
is actually clearer by not using a list and exploding it, so I've done
that and pylint is happy too.
Benoit Pierre