7
\$\begingroup\$

I've written a class to handle my custom errors in Python. The idea is to simplify raising exception with custom messages, all you should do is raise the class and pass it a message.

Everything in the code uses only built-in Python features.

I'd like your feedback on how this implementation could be improved in terms of efficiency and best practices for Python error handling.

Also, I’d like to know if it’s okay to use this approach in my future projects, or if it’s something unnecessary or not recommended in real-world applications.

Here's the code:

import inspect
import linecache

class CustomError(Exception):
    """ 
    msg: custom error message
    
    info: display file/line/func info
    
    """
    def __init__(self, msg:str, info:bool = False):
        super().__init__(msg)
        self.info = info
        frame_current = inspect.currentframe() 
        frame_error = None
        self.line = 0
        self.file_name = "<unknown file>"
        self.func_name = "<unknown func>"
        if frame_current:
            try:
                frame_error = frame_current.f_back 
                code = frame_error.f_code
                self.line = frame_error.f_lineno
                self.file_name = code.co_filename
                self.func_name = code.co_name
            finally:
                del frame_error
                del frame_current
            

        self.source_line = linecache.getline(self.file_name,self.line).strip()
        if not self.source_line:
            self.source_line = "<unavailable source>"
                
                
    def __str__(self):
        base_msg = super().__str__()
        if self.info:
            return (
                f"CustomError: {base_msg}\n"
                f"    File {self.file_name}, line {self.line} in {self.func_name}\n"
                f"    {self.source_line}\n"
                )
        return f"CustomError: {base_msg}"
    
    
    def __repr__(self):
        return(
            f"{self.__class__.__name__}("
            f"msg={super().__str__()!r},"
            f"info={self.info!r},"
            f"file={self.file_name!r},"
            f"line={self.line!r},"
            f"func={self.func_name!r},"
            f"source={self.source_line!r})"
        )
    
    
    
    
# demo (err: empty string)
if __name__ == "__main__":
    def demo_function():
        print("Whats your Name?")
        try:
            user_input = input()
            if user_input == "":
                empty_string_err = CustomError("user input is empty!")
                raise empty_string_err
        except CustomError as e:
            print(f"{e}")
            
    demo_function()
   
\$\endgroup\$
2
  • 1
    \$\begingroup\$ If the info argument is True, then the line printed out is the line on which the CustomError was created and not the line on which it was raised. So wouldn't it be better if those two "events" (creation and raising of the CustomerError) occurred on the same line, i.e. raise CustomError("user input is empty!", info=True)? \$\endgroup\$ Commented Nov 9 at 21:03
  • \$\begingroup\$ Yes, it’s better and I think if I can find a way to display the exact line that caused the error, it would be much better. user_input = input() \$\endgroup\$ Commented Nov 9 at 21:21

3 Answers 3

11
\$\begingroup\$

naming booleans

    def __init__( ... , info: bool = False):

That's just weird. It looks like we're going to have some descriptive information supplied there, and then it turns out it's a flag.

Please use a name like want_info, which suggests it's a flag. Or better, use the conventional name for this: verbose

use Path for a path

        self.file_name = "<unknown file>"

Typing that as str seems unfortunate; better to make it a Path.

Also, if we really need to represent "unknown", then None would be the usual approach. It would indicate that we do not yet have a valid file name.

I do appreciate that the constructor always exits with the same set of object attributes defined, even in the event of an error.

extra refcount decrement

This seems pointless:

                del frame_error
                del frame_current

It looks like java code that assigns frame_current = null; to ensure eventual garbage collection. And yes, I understand we don't want spurious references keeping a frame alive for "too long".

Their refcounts will go to zero in a couple of lines when we return. It's not like it matters whether they exist during the linecache.getline() call. Simply elide the del statements.

spurious try

Checking whether currentframe() gave us None is essentially asking "are we running under cPython?", which seems legitimate. But then the try seems to be trapping AttributeError in case .co_name or similar does not exist. That sounds like a "cannot happen" case to me.

Please use except AttributeError: if that was the true concern.

And in any event the frame_{error,current} refcounts shall be decremented to zero upon leaving this scope, so no need for try / finally.

consistent newline

It seems unfortunate that str(exc) will end with "\n" in the verbose case but not in the default case.


motivation

class PayrollError(Exception):
    pass

class NoFundsError(PayrollError):
    pass

class EmployeeIdNotFoundError(PayrollError):
    pass

or if it’s something unnecessary or not recommended

Generally, I don't get why an app author would want to raise your CustomError. I mean, apps routinely define an app-specific hierarchy such as the example I supplied. Once we have seen that calling code can actually retry / recover from certain conditions, then defining something more specific than ValueError makes sense. And the tree of error classes conveniently lets a caller catch several things or just one.

But the OP code is mostly concerned with inspecting what's on the call stack. By default the interpreter displays a nice backtrace already. It feels like the code we see here belongs up in the app's top-level default error handler which is responsible for logging what happened.

As a maintainer, I feel this code would hide important details from me. When something has gone horribly wrong in an unanticipated way, I really want to see all the details of a backtrace, not just the subset that an app author anticipated would be relevant.

If there is some other audience for these diagnostic messages, such as web client end users, then that should be explicitly written down in """docstrings""" or # comments.

\$\endgroup\$
7
\$\begingroup\$

I have a couple reservations here. In the provided example, CustomError is used as a snap-in replacement for ValueError. The error message already makes it clear where the error comes from, because it is set by the programmer. So the exception handler does not provide a lot of additional insight.

Should you even raise an exception in this case? I find that debatable, because you can easily prevent the error in the first place by checking the value. In a real scenario, you might want to perform even more checks.

That being said, raising an exception is useful when you want to add an entry to the error logs, or when you want some corrective action to take place, like a cleanup routine (even though a context manager or a Finally clause can take care of that).

Minor points

Some statements can be simplified just a little:

self.source_line = linecache.getline(self.file_name,self.line).strip()
        if not self.source_line:
            self.source_line = "<unavailable

Slightly more succinct:

self.source_line = linecache.getline(self.file_name,self.line).strip() or "<unavailable source>"

However, there is a possible flaw here. If linecache.getline returns None, then the .strip you are applying could fail and raise an exception. But wait, let's look at the docs, because I admit I am not familiar with this lib. Sounds like you are in the clear here:

Get line lineno from file named filename. This function will never raise an exception — it will return '' on errors (the terminating newline character will be included for lines that are found).

I believe the frame object properties eg f_lineno etc should all be present, so hopefully no surprises here.

Instead of:

self.file_name = "<unknown file>"
self.func_name = "<unknown func>"

I would use None as a value. That simplifies parsing, and makes it clear that the value is not initialized or not yet known. The programmer retrieving these values from the exception can still reformat the error message as desired.

In __repr__ you use self.__class__.__name__ to identify your class, good idea. You can do the same in the __str__ method.

Stack trace

In more complex situations, the full stack trace is really helpful to have the context of the error, so that the programmer can dig hard into the logs and fix the issue. Perhaps you should consider making it available as an option. The idea should be to add more context and insight, not suppress useful information.

If the intention is really to generalize usage of this class, as an all-purpose exception handler, this could be problematic because we may want to handle exceptions differently, depending on their type. Some exceptions are recoverable, like transient network errors, whereas other exceptions are more fatal and point to a flaw in the program that cannot be easily managed.

\$\endgroup\$
5
\$\begingroup\$

This appears to be a very useful class. As per my comment to your post, I think one would prefer to create the CustomError instance and "raise it" on the same line so that you are reporting where the exception is being raised rather than where the instance was created.

Method CustomError.__init__

Although this method "works", I do think it can be improved so that the logic is a bit clearer and unnecessary initializations are removed. You unconditionally execute the statement ...

self.source_line = linecache.getline(self.file_name,self.line).strip()

... even when there is no current frame and the arguments to linecache.getline would be "<unknown file>" and "<unknown func>". There is not much value in that.

You initialize self.file_name to "<unknown file>" and self.func_name to "<unknown func>" up front, which is an unnecessary initialization when the current frame can be found and these values will be replaced with new assignments. Using an "if-then-else" can improve clarity and efficiency.

If for some reason frame_error = frame_current.f_back raises an exception, then in your finally block, you will be attempting to delete frame_error, which will be None. So you need to check its value before deleting it. And you should move the initialization statement frame_error = None to a position where it makes more sense, i.e. within the if frame_current: block.

Putting it all together:

class CustomError(Exception):
    ...

    def __init__(self, msg:str, info:bool = False):
        super().__init__(msg)
        self.info = info
        frame_current = inspect.currentframe()
        if frame_current:
            frame_error = None
            try:
                frame_error = frame_current.f_back
                code = frame_error.f_code
                self.line = frame_error.f_lineno
                self.file_name = code.co_filename
                self.func_name = code.co_name
                self.source_line = linecache.getline(self.file_name,self.line).strip()
                if not self.source_line:
                    self.source_line = "<unavailable source>"
            finally:
                if frame_error:
                    del frame_error
                del frame_current
        else:
            self.line = "<unavailable source>"
            self.source_line = self.line
            self.file_name = "<unknown file>"
            self.func_name = "<unknown func>"
\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.