Closed tripleee closed 6 years ago
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)
The above push fixed the immediate symptom but this is NOT properly fixed. We should fix the bare except:
clauses and remove the workaround
Question: Are there other Python linters that we can keep our settings with, that are slightly less pedantic about everything?
pep8
is the bare minimum, it's not nearly as detailed as flake8
of course.
Retitled this, as the immediate flake8
breakage was already worked around, while the root cause remains.
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
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.
I would argue that this issue isn't fully finished until all the except Exception
s are replaced with their specific Exceptions.
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:
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-isnocrash.py
: It's followed by a except BaseException
so KeyboardInterrupt
s and SystemExit
s are correctly caught. 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
: hfgfhfgfhfghghgfgghfgfhfgfhfgWebSocketException
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.
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 bareexcept:
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,
... should probably be refactored to not use a bare
except:
in the inner scope, and possibly should not usetry
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 themraise
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).