Two versions of user-specified exception filters that log a different message on match

Question

I'd like to show you two versions of my two functions that support my logging tools. Their purpose is to log a different message (here abort) when an error occured and it matches the filter specified by the user. By default the error message is logged.

Version-1 of this code uses the classical if to handle the optional filter, while version-2 uses the a try/except.

Classical if

The first function is an optional filter that logs an abort trace when the exception matches any of the error types specified by the user and returns True in this case, otherwise False:

class LogAbortWhen:
    def __init__(self, *error_types: type[Exception]):
        self.error_types = error_types

    def __call__(self, exc: Exception, logger: TraceLogger) -> bool:
        if any((isinstance(exc, t) for t in self.error_types)):
            logger.final.log_abort(message=f"Unable to complete due to <{type(exc).__name__}>: {str(exc) or '<N/A>'}")
            return True
        return False

The other one logs the default error trace when there is no custom filter or the filter returned False:

@contextlib.contextmanager
def telemetry_context(
        activity: str,
        source: Source,
        on_error: Optional[OnError] = None,
) -> ContextManager[TraceLogger]:  # noqa
    parent = current_logger.get()
    logger = BasicLogger(activity)
    tracer = TraceLogger(logger)
    tracer.initial.source.append(source)
    token = current_logger.set(Node(logger, parent))
    try:
        yield tracer
    except Exception as e:  # noqa
        if not on_error or not on_error(e, tracer):
            tracer.final.log_error(message=f"Unhandled <{type(e).__name__}> has occurred: <{str(e) or 'N/A'}>")
        raise
    finally:
        current_logger.reset(token)

---

Easier to ask for forgiveness than permission: try/except

But then I thought, how about rewriting it with the easier to ask for forgiveness than permission principle in mind, so I did the following (theoretical code):

The LogAbortWhen2 now throws the DoesNotApply error when it didn't do anything:

class DoesNotApply(Exception):
    pass


class LogAbortWhen2:
    def __init__(self, *error_types: type[Exception]):
        self.error_types = error_types

    def __call__(self, exc: Exception, logger: TraceLogger):
        if any((isinstance(exc, t) for t in self.error_types)):
            logger.final.log_abort(message=f"Unable to complete due to <{type(exc).__name__}>: {str(exc) or '<N/A>'}")
        else:
            raise DoesNotApply()

And the context-manager telemetry_context2 doesn't check the return value anymore, but handles the two errors that may occur: TypeError | DoesNotApply. (the TypeError is for when the on_error variable is None; I know, I should implement an empty empty filter always throwing the DoesNotApply error)

@contextlib.contextmanager
def telemetry_context2(
        activity: str,
        source: Source,
        on_error: Optional[OnError] = None,
) -> ContextManager[TraceLogger]:  # noqa
    parent = current_logger.get()
    logger = BasicLogger(activity)
    tracer = TraceLogger(logger)
    tracer.initial.source.append(source)
    token = current_logger.set(Node(logger, parent))
    try:
        yield tracer
    except Exception as e:  # noqa
        try:
            on_error(e, tracer)
        except TypeError | DoesNotApply:
            tracer.final.log_error(message=f"Unhandled <{type(e).__name__}> has occurred: <{str(e) or 'N/A'}>")
        raise
    finally:
        current_logger.reset(token)

---

Which version would you prefer? The first one with the bool return value and the additional if or the second one with the try/except? Also how strict are you about the EAFP principle? Do you often write your code if-free and favor try/excepts?

Answer 1

• You're not running your static typing tool in strict mode, limiting the benefits to static typing your code.

• When to use *args can be a bit of a tricky one. However if you're ok with taking a tuple[type[Exception], ...] then we can use dataclasses which can automatically build a number of dunders for you.

  Additionally I think having the class be frozen (immutable) makes sense, and you can automatically make __slots__ for increases in performance and memory usage.

• I think you want to use BaseException not Exception. Please familiarize yourself with Python's exception hierarchy, which shows all exception inherit from BaseException.

• isinstance takes type[BaseException] | tuple[type[BaseException], ...] as the type argument, meaning you don't need to use any and a generator expression.

@dataclasses.dataclass(frozen=True, slots=True)
class LogAbortWhen:
    exceptions: tuple[type[BaseException], ...]

    def __call__(self, exc: BaseException, logger: TraceLogger) -> bool:
        if isinstance(exc, self.exceptions):
            logger.final.log_abort(message=f"Unable to complete due to <{type(exc).__name__}>: {str(exc) or '<N/A>'}")
            return True
        return False


# Example usage
abort = LogAbortWhen((KeyboardInterrupt, OSError))
assert abort(OSError("test error"), TraceLogger(...))

• Personally I like to use a single level of indentation for arguments to functions. I have always found the ): and now ) -> ...: to be enough visual separation.

• The parent to token section of the function doesn't seem to be using standard Python, so commenting is hard. However I don't think the code belongs in the function.

  Why can't BasicLogger have a method to build and configure the TraceLogger?

• I'm not sure what OnError looks like. However I imagine the class will look quite similar to LogAbortWhen and I'm concerned you have a lot of WET code and possibly some over engineering.

Overall I don't have too much to say at a 'micro' level. However at a more macro level I'm unsure why you don't add a context manager to LogAbortWhen.

@dataclasses.dataclass(frozen=True, slots=True)
class LogAbortWhen:
    exceptions: tuple[type[BaseException], ...]

    def log(self, exc: BaseException, logger: TraceLogger) -> bool:
        logger.final.log_abort(message=f"Unable to complete due to <{type(exc).__name__}>: {str(exc) or '<N/A>'}")

    def __call__(self, exc: BaseException, logger: TraceLogger) -> bool:
        if isinstance(exc, self.exceptions):
            self.log(exc, logger)
            return True
        return False

    @contextlib.contextmanager
    def handle_error(self, logger: TraceLogger) -> ContextManager[None]:
        try:
            yield None
        except BaseException as e:
            if isinstance(e, self.exceptions):
                self.log(exc, logger)
            raise


with LogAbortWhen((OSError,)).handle_error(TraceLogger(...)):
    raise OSError("test error")


@contextlib.contextmanager
def telemetry_context(
    activity: str,
    source: Source,
    on_error: Callable[[TraceLogger], ContextManager[Any]] = lambda t: contextlib.nullcontext(enter_result=None),
) -> ContextManager[TraceLogger]:  # noqa
    parent = current_logger.get()
    logger = BasicLogger(activity)
    tracer = TraceLogger(logger)
    tracer.initial.source.append(source)
    token = current_logger.set(Node(logger, parent))
    try:
        with on_error(tracer):
            yield tracer
    finally:
        current_logger.reset(token)


with telemetry_context(..., on_error=LogAbortWhen((OSError,))) as tracer:
    pass

Comparative Review

A classic LBYL/EAFP example is reading from a file.

# LBYL
if path.exists():
    with open(path) as f:
        data = f.read()

# EAFP
try:
    f = open(path)
except OSError:
    pass
else:
    with f:
        data = f.read()

The difference is one calls path.exists first. However, EAFP is better. The LBYL code has a race condition, if the file doesn't exist when path.exists() is called and is made before open(path) then we will need to handle an OSError.

The bug free version of LBYL is just EAFP with more code:

if path.exists():
    try:
        f = open(path)
    except OSError:
        pass
    else:
        with f:
            data = f.read()

Now lets look at your code:

# LBYL
try:
    yield tracer
except Exception as e:  # noqa
    if not on_error or not on_error(e, tracer):
        tracer.final.log_error(message=f"Unhandled <{type(e).__name__}> has occurred: <{str(e) or 'N/A'}>")
    raise
finally:
    current_logger.reset(token)

# EAFP
try:
    yield tracer
except Exception as e:  # noqa
    try:
        on_error(e, tracer)
    except TypeError | DoesNotApply:
        tracer.final.log_error(message=f"Unhandled <{type(e).__name__}> has occurred: <{str(e) or 'N/A'}>")
    raise
finally:
    current_logger.reset(token)

Your EAFP is just LBYL but with more code. So isn't a good use of EAFP.