Run two different flake8 configs

[cclauss]

Related to #1438 (comment)

This will allow us to remove ignored tests gradually over multiple PRs that are easier to review and merge.

Proposed Changes

Run two flake8 commands: 1) Just /rdflib, 2) The whole repo

  - flake8 --ignore=E203,E266,E402,E713,E722,E741,F401,F402,F403,F811,F821,F841,W503
           --max-complexity=41 --max-line-length=1656 --show-source --statistics rdflib
  - flake8 --ignore=E101,E126,E203,E265,E266,E303,E402,E711,E712,E713,E722,E731,E741,F401,F402,F403,F405,F523,F541,F811,F821,F841,W191,W291,W293,W503
           --max-complexity=41 --max-line-length=1653 --show-source --statistics .

[nicholascar]

I like this idea, but can you please provide a bit more info as to why certain tests have been selected to be ignored? For example, why is E741 ignored? I'm not saying I disagree with it being ignored, only we need why certain tests are ignored to be documented. Perhaps the reasons are already documented but I don't know where?

[cclauss]

This PR is trying to set us on a path of continuous improvement where contributors can pick a few codes and fix all issues to those codes. Fixing all failing codes in a single PR will be difficult to review and by the time that PR is approved, there will already be git conflicts with other changes landed in the meantime. The reason for ignoring E741 is documented under flake8 --select=E741 --show-source --statistics which finds 26 instances of this issue. If I were to fix all of them in this PR then it is far less likely that this PR would be reviewed and landed. Note that formatting this codebase with psf/black would autofix many of the flake8 style errors.

[nicholascar]

Ok, I understand: you wish to suppress tests initially then incrementally test for them and fix them.

Note that formatting this codebase with psf/black would autofix many of the flake8 style errors.

This cannot completely be the case as the entire code base is formatted with black and yet we still have issues! We have been using black since 5.0.0. Are we perhaps not using back with all style fixtures turned on in some way?

@ashleysommer can you please comment on this PR and the black assertion?

[cclauss]

the entire code base is formatted with black

Not quite. The drone command is black --config black.toml --check ./rdflib || true

• The || true means that command can fail and drone will still pass.
• The ./rdflib bit means that black is only checking part of the repo.
• black.toml contains a bunch of other exclusions.

psf/black would autofix many of the flake8 style errors.

• many is not all.
• Not all flake8 errors are style errors. For example flake8 --select=E9,F63,F7,F82 are not style errors.

[nicholascar]

OK, I understand now. We do format with black locally too, and I've run black on /test as well as /rdflib. So all of the repo has been black formatted recently, even if it's not being done with each Drone run.

I'm ok with accepting your direction here, as long as @ashleysommer & @white-gecko are fine with the Drone changes as they really maintain that!

[aucampia]

I would rather leave this in place unless it is causing specific problems. Otherwise black and flake8 will fight with each other about line length and I think it is better to have line length controlled by black than have it controlled by flake8.

The ideal is to eventually have everything compliant with flake8 config as per .flake8. I check all patches I make with:

darker test/ rdflib/
git diff | flake8 --diff

And without this I will get errors about line length. I can manage this locally also if it has to be but I think the benefit of having this here is it kind of sets a shared goal/ideal of where we want to get to in terms of flake8.

[cclauss]

black is best-effort while flake8 is pass/fail. flake8 must be told that line length is 88 or it will assume its default value of 79. Black will try to make lines 88 characters but it will not flag lines that are longer than 88 characters that it can not autoformat to be 88 or less. So, the advantage of flake8 monitoring the line length is that it will be the squeaky wheel that asks the contributor to make a manual correction while black will not.

% cat really_long_line.py

#        1         2         3         4         5         6         7         8         9
# 3456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789

% black really_long_line.py

All done! ✨ 🍰 ✨
1 file left unchanged.

% flake8 --max-line-length=88

./really_long_line.py:1:89: E501 line too long (90 > 88 characters)

[aucampia]

But this is why the config disables E501 (the error raised in your example). The aim is to defer line length solely to black, so that black will fail when it is unhappy, but flake8 won't care about line length (except if you have flake8-black in which case it will tell you that black is unhappy).

With no .flake8 config, if I develop on my machine, the default flake8 config will apply, which will complain a lot about line length, and this will not work well:

darker test/ rdflib/
git diff | flake8 --diff

because darker will use black's logic to determine line length, and flake8 will think it is wrong.

[aucampia]

The other option is to put max-line-length=1656 in the config (i.e. .flake8) but I'm not sure that is better because effectively that disables E501 for most practical cases, and it may just be better to not have the check to begin with.

[cclauss]

Best practice is to work on decreasing the max-line-length with warnings about any transgressions instead of continuing to ignoring all test results with —exit-zero and || true.

[aucampia]

I'm not arguing we should keep --exit-zero or || true. I'm saying we should decide if black will control the line length or flake8, and we should then configure .flake8 accordingly. Like I said, if you feel that max-line-length=1656 makes more sense in .flake8 than ignoring E501 that is fine, but if you put neither down, then flake8 and black will be trying to do two very different things.

[aucampia]

The benefit I feel that ignoring E501 has over trying to clamp the line length with flake8 is that black will already clamp line length for code while E501 will make flake8 complain about comments also, and in some cases URLs in comments exceed the 80 default line length and it is quite annoying to have to work around this. But even so, if you want to put the recommended settings for black in .flake8 I will be fine with that also.

But not putting anything in config does not resolve the inherent disagreement that flake8 and black has about line length by default.

[nicholascar]

@cclauss I see you've also closed this PR. I hope you've not decided to keep supporting the improvements you've suggested here! We may have to work out details still, such as the points about line length above, but it doesn't mean we don't want the PR merged in some form. Or perhaps you're shifting some of these edits to another PR?