flake8: simplify

[oliver-sanders]

More meeting code fiddling, closes #4239

List of simplify codes here: https://github.com/MartinThoma/flake8-simplify#SIM106

Used a few noqa flags where appropriate, there are a few things in the code that are hard to refactor, usually patterns which should be avoided but which aren't worth the pain of totally overhauling at this stage.

Suggest adding ?=1 to the diff URL to hide the whitespace changes.

Requirements check-list

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Applied any dependency changes to both setup.py and
  conda-environment.yml.
• Already covered by existing tests.
• No change log entry required (why? e.g. invisible to users).
• No documentation update required.

[oliver-sanders]

We could combine these two with statements into a single one, however, since all of the others in this file are written individually this would break the pattern somewhat.

[oliver-sanders]

with suppress(Something):
    something()

Is generally cleaner and more readable than:

try:
    something()
except Something:
    pass

I'm not so fussed, if this causes us undue pain we can always disable the rule, however, it does have a potential advantage from the perspective of code coverage so thats nice.

[hjoliver]

I quite like the with suppress... version.

[MetRonnie]

One thing to bear in mind is with suppress is slower, so we might want to avoid it sometimes

from contextlib import suppress

def f():
    try:
        int('a')
    except ValueError:
        pass
    
def g():
    with suppress(ValueError):
        int('a')

In [5]: %timeit f()
976 ns ± 46 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [6]: %timeit g()
1.63 µs ± 32.8 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

[oliver-sanders]

flake8-simplify wants you to handle the error cases first, so this:

if error_cond:
    raise Exception()
something()

Rather than this:

if not error_cond:
    something()
else:
    raise Exception()

This is generally a good idea, however, I think for pattern-matching type problems where you have a list of conditions and handlers it's a good pattern to have a else: raise catchall at the end:

if cond_a:
    something('a')
elif cond_b:
    something('b')
...
elif cond_z:
    something('z')
else:
    raise Exception()

So I've left these example in with noqa flags.

[oliver-sanders]

Needlessly nested if's

[oliver-sanders]

Python2 class.

[oliver-sanders]

with open() is the better pattern.

Note that this:

with open() as h:
    h.write()

Is not equivalent to this:

h = open()
h.write()
h.close()

As the with open brings extra error handling to the table.

[oliver-sanders]

any is faster and more readable.

[oliver-sanders]

flake8-simplify is keen on dataclasses.

I.E. this:

from dataclasses import dataclass

@dataclass
class Foo:
    a: str
    b: str

Rather than this:

class Foo:

    def __init__(self, a: str, b: str):
        self.a = a
        self.b = b

Dataclasses have additional benefits because they add other methods like __str__ and __eq__ which saves a lot of boilerplate of thekind we are often to lazy to write.

Dataclasses are generally a good idea, however, a lot of our classes (like this one) don't fit the pattern so well.

Nice to be reminded that we could use a dataclass, just stick a noqa in if not appropriate.

[hjoliver]

👍 Quite a big change but just straightforward switch to other valid coding constructs, so one review will do on this.

[MetRonnie]

Is this not going to be added to GH Actions? Ah wait, it's run as part of the normal flake8 command of course

[oliver-sanders]

(we can use the flake8 --select option to make it more explicit, otherwise flake8 runs all installed flakes)