Updating the errors we ignore with flake8

I’m going through the ignore errors on flake8, and noticed that there are some that I don’t quite understand why we are choosing to ignore.

  1. Continuation line under-indented for hanging indent (E121)
    • There’s only a single JSON in the codebase that doesn’t comply with this already.
  2. Closing bracket does not match indentation of opening bracket’s line (E123)
    • This one could go either way via PEP, but it may be worth just enforcing a standard forconsistency. The vast vast majority of our code already conforms to the “best practice”.
  3. Continuation line over-indented for hanging indent (E126)
    • Similar to E121, there’s only a handful of lines that don’t follow this already and is just enforcing 4 space indentation levels.
  4. Multiple spaces before operator (E221)
    • This one seems like a no brainer to implement. Our current reasoning is for “horizontal alignment” but there is only one line of code in the entire codebase that takes advantage of it.
    • Relevant PR
  5. Missing whitespace around arithmetic operator (E226)
    • Good news, there isn’t a single line of code that doesn’t already conform to this. Might as well make it official.
    • Relevant PR
  6. Tab after ‘,’ (E242)
    • Again, nothing in our codebase goes against this so might as well make it official.
    • Relevant PR
  7. Expected 2 blank lines after end of function or class (E305)
    • This seems like standard spacing (and per PEP)
  8. Multiple statements on one line (def) (E704)
    • Another one where we don’t have any current errors for this one. Seems reasonable to create a standard for it.
    • Relevant PR
  9. Do not use variables named ‘I’, ‘O’, or ‘l’ (E741)
    • There’s about ~20 lines of code that don’t conform to this. Wouldn’t be too hard to fix those and enforce this one.
  10. Line break occurred after a binary operator (W504)
    • This one used to used to be the other way around, but it seems PEP has taken an official stance and prefers this rule. Makes sense to make a standard in our codebase. (would require changing ~40 lines of code)

Thoughts? Objections? If these are seen as reasonable changes, I’ll make a PR implementing them.

Cool; thanks for the audit! Many of these probably date from when we first enabled flake8 in CI and prioritized efficiently “getting to green.” Certainly we can immediately rip out those no-ops. The rest deserve some case-by-case thought but we should at least take a look at the impact on the code in a diff.

1 Like

Happy to tackle if @jtpavlock has not already completed it.

Planning on kicking this off later today, I’ll let you know when I post the PR!

Already started. :slight_smile:

Hey, sorry I should have clarified I had already had the initial PR pretty much done when I commented, I just was encountering some VM issues preventing me from pushing to github.

I posted the first PR https://github.com/beetbox/beets/pull/3666

But, I didn’t include any of the ones that involve any codebase changes > 2 lines. Feel free to tackle any of those! I added a list of remaining error codes we’d like to see fixed on the PR

Pulled your PR and working my way through.

Completed, but not sure how to push to your repo as I have no permission to publish to your repo.

Created a PR against your branch.

1 Like