Smokedetector: Remove bare except's and document broad exceptions.

[tripleee]

I'm speculating that Travis changed something because the build which is failing is just a trivial blacklist edit.

https://chat.stackexchange.com/transcript/message/40709387#40709387

flake8 chokes on bare except: clauses and I certainly agree that we should use a _lot_ fewer of these -- ideally none at all.

(The rest is a rant. You might want to stop reading here.)

If we have to have them, there should be a maximum of one per call tree, and as far up the call chain as possible. For example,

try:
  something
  try:
     something else
  except:
     pass
except:
  pass

... should probably be refactored to not use a bare except: in the inner scope, and possibly should not use try at all there as the exception will be captured by the caller anyway, and this sort of structure is extremely hard to understand and debug (and the real-life cases will be much more complex than the above).

As a rule of thumb, I like to put the ugliest code in main and keep the internal functions as clean and obvious as possible, and even have them raise errors when they need to (trusting that the ugliness of coping with the exception needs to be specific to each caller anyway, but we can keep things simple and elegant inside library functions).

[AWegnerGitHub]

Not a Travis thing. flake8 itself updated and we aren't pinned to a specific version.

Release notes from 3.5.0: http://flake8.pycqa.org/en/latest/release-notes/3.5.0.html

Start using new PyCodestyle checks for bare excepts and ambiguous identifier (See also GitLab#361)

[tripleee]

The above push fixed the immediate symptom but this is NOT properly fixed. We should fix the bare except: clauses and remove the workaround

[ArtOfCode-]

Question: Are there other Python linters that we can keep our settings with, that are slightly less pedantic about everything?

[tripleee]

pep8 is the bare minimum, it's not nearly as detailed as flake8 of course.

[tripleee]

Retitled this, as the immediate flake8 breakage was already worked around, while the root cause remains.

[iBug]

I think I've replaced all that I can. Here are the rest of them as of f701e90c5217b3cdb385c522720d6bdd53ce422c. Don't forget to remove E722 from tox.ini before running flake8.

./chatcommands.py:1418:5: E722 do not use bare except'
./chatcommands.py:1441:9: E722 do not use bare except'
./chatcommands.py:1500:5: E722 do not use bare except'
./datahandling.py:427:13: E722 do not use bare except'
./deletionwatcher.py:32:9: E722 do not use bare except'
./deletionwatcher.py:96:13: E722 do not use bare except'
./metasmoke.py:60:25: E722 do not use bare except'
./metasmoke.py:62:13: E722 do not use bare except'

Also, there are some other places where I just changed it into except Exception:. I left a comment for every single except Exception so they're "documented" to some extents. Some are commented with a TODO when I believe they should be replaced for more accurate exception groups instead of being left as-is. You can easily find them out with:

grep -nF 'except Exception:', *.py

[iBug]

Finally replaced all bare except's, although some of them were not done in the best manner and were just inserted a Exception. I've left a comment on every single except Exception so they can be easily understood.

P.S. E722 is no longer ignored, so further bare except's will make flake8 angry.

[angussidney]

I would argue that this issue isn't fully finished until all the except Exceptions are replaced with their specific Exceptions.

[iBug]

So then, since it's raised, I'll leave this comment here.

As of revision ee2a44d4afcf9e31e96131608706ab6e4e8b631b, these broad exceptions exist.

wsl@MSI-GS70:~/proj/SmokeDetector $ grep -nF 'except Exception:' *.py
chatcommands.py:1418:    except Exception:  # I don't want to dig into ChatExchange
chatcommands.py:1441:        except Exception:  # I don't want to dig into ChatExchange
chatcommands.py:1500:    except Exception:  # I don't want to dig into ChatExchange
chatcommunicate.py:354:            except Exception:  # Everything else
excepthook.py:55:            except Exception:  # Broad exception makes sense here
fix_pickles.py:19:        except Exception:  # TODO: What could happen here?
metasmoke.py:311:            except Exception:  # TODO: What could happen here?
nocrash.py:87:    except Exception:  # Kinda sorta intended, used to be a pass so the BaseException below only catch program exits
spamhandling.py:161:    except Exception:  # TODO: What could happen here? We don't want unnecessary `except Exception`s
ws.py:177:    except Exception:  # TODO: What could happen here?

Every single non-capturing broad exception explained:

• The three in chatcommands.py: The only statement inside the try block is a msg.delete() call. One needs to be familiar with ChatExchange to determine what could actually be raised here, but I'm not.
• chatcommunicate.py: It's appropriate here, to catch all exceptions raised from chat command functions (except CmdException which will have its message returned).
• excepthook.py: Should be obvious that it's appropriate here.
• fix_pickles.py: I remember that it's already replaced with OSError in the up-to-date master branch ...?
• metasmoke.py: Not sure what could be raised, left as-is
• nocrash.py: It's followed by a except BaseException so KeyboardInterrupts and SystemExits are correctly caught. ~Could be~ Already done in a more elegant way.
• spamhandling.py: Looks like it's intended (I left a wrong comment) so that any exception popped up in handle_spam() is handled by the exception hook.
• ~ws.py: hfgfhfgfhfghghgfgghfgfhfgfhfg~ Replaced with WebSocketException already.

There are another 12 except Exception as e's that all seems to be correctly handled (logged and/or force reboot) and don't belong to this topic.

To sum up, what needs to be replaced by more specific exceptions are the 3 in chatcommands.py and the one in metasmoke.py. This time it really should be the last of them.