Update flake8 configuration

[ewu63]

Purpose

This PR updates the flake8 configuration:

• added the following plugins and checks
  • flake8-bugbear
  • flake8-comprehensions
  • flake8-docstrings
  • flake8-builtins
  • flake8-rst-docstrings
• updated the flake8 config file to reflect those changes
• switched to using requirements files for pinned versions of python packages

I have tested the config file locally, but we can only test Azure after merging this. I wouldn't be surprised if something broke and I will try to patch it after.

Everything here is up for discussion, I know this is rather intrusive so happy to change/reduce the scope if needed.

[A-CGray]

Could you explain what these new flake8 checks will catch and what the reason for switching to requirements files is?

[ewu63]

Could you explain what these new flake8 checks will catch and what the reason for switching to requirements files is?

• flake8-bugbear catches common bad coding practices. This IMO is the most important one out of these plugins (it's also maintained by pyCQA so it's maybe more reputable). See here for a list of things they catch.
• flake8-comprehensions is for improving dict/set/list comprehensions. Not bugs or anything, just improvement in code quality and making things more pythonic.
• flake8-builtins checks for instances where variable names overshadow a python builtin, such as set = 10. In most cases this is probably okay, but still considered kinda dangerous IMO.
• flake8-docstrings is another plugin from pyCQA, it adds pydocstyle to flake8 so we can lint docstrings according to a given convention (in this case numpydoc). It will complain if there is missing docstring, or if a parameter is missing or anything like that. This check is currently disabled because very few of our docstrings are up to standard.
• flake8-rst-docstrings is for linting RST compliance with our docstrings, whereas the previous plugin just checks for plaintext. I actually think this is probably not that necessary since the plugin is also very much in alpha/beta status. One related thing we could do instead (and I've talked to @eirikurj about this) is to make sphinx build warnings into errors on RTD instead.

I moved the installation to a requirements file to manage the environment a bit more easily, particularly if we want to pin specific versions of packages (as we do now with flake8). I didn't like having those specified in the command line in a YAML file somewhere. I will remove the black requirements file as per the discussion above, but I think the flake8 stuff can probably benefit from this sort of partition.

[marcomangano]

One related thing we could do instead (and I've talked to @eirikurj about this) is to make sphinx build warnings into errors on RTD instead.

Can you clarify this? We want sphinx to fail if the linting is not RST compliant? Is this to avoid/minimize weird renderings on RTD?

[ewu63]

Can you clarify this? We want sphinx to fail if the linting is not RST compliant? Is this to avoid/minimize weird renderings on RTD?

Not the linting, but when building sphinx docs, sometimes we get a bunch of warnings (such as target not found, section heading inconsistent, etc.) but those are not reported by RTD. If you are not checking the logs you have no idea anything was wrong at all. The idea would be to change the makefile for sphinx such that it fails on warnings.

[marcomangano]

Oh, I like this, although we have to be careful about which warnings to include, some of them are pretty harmless, others are expected to pop up locally (e.g. links to private repo docs)

[ewu63]

Oh, I like this, although we have to be careful about which warnings to include, some of them are pretty harmless, others are expected to pop up locally (e.g. links to private repo docs)

This is a good point, I tried to mock something up but could not bypass this without leaking the RTD tokens in PRs. Let's shelve this for now, and hopefully people who are editing docs are examining their build logs (either locally or on RTD).

[marcomangano]

Alright, so are we converging on using flake8-bugbear and flake8-builtins? I am also fine with including flake8-comprehensions

[ewu63]

I personally find flake8-comprehensions quite useful and will continue to use it locally, but I also agree with Ali that it's not strictly necessary so I can go either way on that.

As for documentation updates, again I've disabled flake8-docstrings since most of our docstrings are either missing or incomplete. But the RST one is still helpful for linting what little docstrings we have.

So, let's come to a decision on the list comprehension stuff and we can proceed with this PR. I would also like to hear your opinions on this @eirikurj and @sseraj.

[sseraj]

In general, I agree with Ali that we should not be overly strict on style. However, looking at the rules for flake8-comprehensions, I think it makes sense to add. It only comes into play if you choose to use comprehensions, and the rules focus on removing unnecessary syntax.

[ewu63]

Forgot to mention that I also added flake8-broken-line to forbid hard line wrapping in Python via \. Don't think many people are adding them, but a lot of legacy code has them everywhere and they are super annoying to deal with.

[A-CGray]

To be clear I don't think flake8-comprehensions is useless I will add it to my own local flake8 config, I just don't want us to have to waste a bunch of time going through thousands of lines of old code to fix things that aren't technically broken in order to get our formatting checks to pass again. Is there a corresponding formatting tool that will automatically fix this kind of thing?

[sseraj]

I just don't want us to have to waste a bunch of time going through thousands of lines of old code to fix things that aren't technically broken in order to get our formatting checks to pass again.

This is a good point.

Forgot to mention that I also added flake8-broken-line to forbid hard line wrapping in Python via .

Doesn't black take care of this?

[ewu63]

Yeah okay that makes a lot of sense. Let's do this in steps then, for now just enable things that point out bugs, and deal with more formatting/code enhancement items later. How does this look?

Doesn't black take care of this?

I think it does but not when used to break strings into multiple lines (which is the main case remaining).

[A-CGray]

Yeah okay that makes a lot of sense. Let's do this in steps then, for now just enable things that point out bugs, and deal with more formatting/code enhancement items later. How does this look?

Is there a way we can add the non-bug plugins as non-blocking checks? Or is that more effort than it's worth?

[ewu63]

Is there a way we can add the non-bug plugins as non-blocking checks? Or is that more effort than it's worth?

Don't think it's worth it:

• it will take work to set up additional jobs
• they will still cause the Azure badge to say "partially succeeded"
• I doubt people are checking those logs anyways if we are not enforcing anything