Add flake8 check for blind excepts

[jackwilsdon]

Add flake8 check for blind excepts using flake8-blind-except, as suggested by @sampsyo in #1957.

Example of invalid code (copied from flake8-blind-except documentation):

try:
    something_scary()
except:
    everybody_panic()

Example of valid code (copied from flake8-blind-except documentation):

try:
    something_terrifying()
except TerrifyingException:
    dont_panic()

In it's current state (as of 698203e), this check is not passed by beets.

Here are the offending files and line numbers:

beets/mediafile.py:1213:13: B901 blind except: statement
beets/plugins.py:267:9: B901 blind except: statement
beets/util/bluelet.py:285:9: B901 blind except: statement
beets/util/bluelet.py:382:9: B901 blind except: statement
beets/util/functemplate.py:527:9: B901 blind except: statement
beets/util/pipeline.py:262:9: B901 blind except: statement
beets/util/pipeline.py:310:9: B901 blind except: statement
beets/util/pipeline.py:349:9: B901 blind except: statement
beets/util/pipeline.py:417:9: B901 blind except: statement
beetsplug/duplicates.py:187:13: B901 blind except: statement
beetsplug/thumbnails.py:274:9: B901 blind except: statement
beetsplug/thumbnails.py:285:9: B901 blind except: statement
test/test_logging.py:258:9: B901 blind except: statement
test/test_replaygain.py:53:9: B901 blind except: statement

Should I fix these in this PR too?

[wisp3rwind]

The first commit in #1957 does nothing but fix these (except for the ones in bluelet.py, which I was not sure about. They should probably be except BaseException, at least there's some KeyboardInterrupt handling). Feel free to cherrypick this into master (@sampsyo already suggested this), would have done that anyway.

[sampsyo]

This is awesome; thank you! ✨ Yes, let's cherry-pick the fixes from @wordofglass and then we can thi carefully about the Bluelet handlers.

[wisp3rwind]

@jackwilsdon: Have you already done this locally? If not, I'd rebase this on master later and push the other changes.

[jackwilsdon]

Have I cherry-picked your changes locally? I haven't had chance yet but I'll do it in a moment 👍.

[wisp3rwind]

You'll want to cherry-pick aa4b697 and 0c19683

[jackwilsdon]

So it looks like we still have some files not passing the check:

test/test_logging.py:258:9: B901 blind except: statement
test/test_replaygain.py:53:9: B901 blind except: statement
test/test_replaygain.py:60:13: B901 blind except: statement

Taking a look at these files, I'm not entirely sure what exceptions could be thrown. Maybe @wordofglass or @sampsyo has a better insight into the code?

Do we need to catch a ReplayGainError on test/test_replaygain.py:53?

On a semi-related note, is there a reason FatalReplayGainError is not a subclass of ReplayGainError?

[wisp3rwind]

The one in test_logging seems fine as it only prints some debugging information and re-raises. So to silence the warning, I'd change that to except BaseException or except Exception, I'm not really sure which is better. KeyboardInterrupt or SystemExit are probably not the primary reason for printing this information, on the other hand, if the test should lock up at some point, that might be useful.

The same goes probably for test_replaygain. Or there the checks might not be necessary at all, because the test module skips any tests when the backends are not available. That should in turn prevent load_plugins from failing in the first place.

In the end, I think in tests this really does not matter as much. When they start failing, you might need to put in some more debugging code anyway.

(I do not know these plugins, the above are first thoughts after having looked at the code for a few minutes.)

[jackwilsdon]

Should I just whack some # noqa comments in the tests then?

[wisp3rwind]

IMHO, naming Exception/BaseException explicitely is preferable as that way it seems more obvious what you want to do than with # noqa, and (hopefully at least), someone who catches BaseException and writes that out has thought about the catch-all beforehand. That's not really a strong opinion, though.

[sampsyo]

Yep, all of these changes in the tests look right to me.

[wisp3rwind]

I just noticed that this never got merged. @jackwilsdon, are you still in on it? Otherwise I'd rebase and push it.

[sampsyo]

Good point, @wordofglass—this looks ready now, modulo a rebase.

[wisp3rwind]

Rebased and committed to master (and fixed a (probably only recently introduced) except: that was not yet included in this PR)

[jackwilsdon]

I'll close this up then, thanks!