I split this issue from #6 since that turned out to be a separate cause.
In my branch for PR #41 I left bare except (except:) statements in the code since the handlers seem to provide behaviors that are expected: Typically, someone will want to discard bad data from the network rather than crashing the network stack, so bare except seems ok. For example, in my branch I have:
try:
retval = ControlFrame((frame.header >> 12) & 0x2FFFF)
return retval # top 1 bit for out-of-band messages
except KeyboardInterrupt:
raise
except:
logging.warning("Could not decode header 0x{:08X}"
"".format(frame.header))
return ControlFrame.UnknownFormat
However, if there are clear ways to identify all exceptions, some code can be made easier to diagnose such as showing errors to the caller. Crashing in that case is better since it allows diagnosis of issues that are code rather than data related. It also reduces the code necessary in contrast to the fix above (using positive rather than negative [bare except] identification):
try:
retval = ControlFrame((frame.header >> 12) & 0x2FFFF)
return retval # top 1 bit for out-of-band messages
except ValueError:
logging.warning("Could not decode header 0x{:08X}"
"".format(frame.header))
return ControlFrame.UnknownFormat
which would allow Python to raise KeyboardInterrupt since there is no bare except. PEP8 recommends handling specific exceptions rather than bare except, but in the cases I've seen here it seems fine unless there are cases like this where the bad data can be clearly identified. One reason to do it this way rather than bare except is that in case frame is None or the header member is missing or None we could diagnose that AttributeError rather than hiding it. In that case, raising the exception would be better, because it would not actually be a UnknownFormat but rather a logic error (or error that should have been handled before passing a bad object along) that is in the code rather than the data.
Therefore I suggest this change if it looks ok, and possibly re-evaluating each bare except (instances of except:).
I split this issue from #6 since that turned out to be a separate cause.
In my branch for PR #41 I left bare except (
except:) statements in the code since the handlers seem to provide behaviors that are expected: Typically, someone will want to discard bad data from the network rather than crashing the network stack, so bare except seems ok. For example, in my branch I have:However, if there are clear ways to identify all exceptions, some code can be made easier to diagnose such as showing errors to the caller. Crashing in that case is better since it allows diagnosis of issues that are code rather than data related. It also reduces the code necessary in contrast to the fix above (using positive rather than negative [bare except] identification):
which would allow Python to raise KeyboardInterrupt since there is no bare except. PEP8 recommends handling specific exceptions rather than bare except, but in the cases I've seen here it seems fine unless there are cases like this where the bad data can be clearly identified. One reason to do it this way rather than bare except is that in case frame is None or the header member is missing or None we could diagnose that AttributeError rather than hiding it. In that case, raising the exception would be better, because it would not actually be a UnknownFormat but rather a logic error (or error that should have been handled before passing a bad object along) that is in the code rather than the data.
Therefore I suggest this change if it looks ok, and possibly re-evaluating each bare except (instances of
except:).