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 inspectimport linecacheclass 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()
coderodde's user avatar
coderodde
32.3k15 gold badges79 silver badges205 bronze badges
askedNov 9 at 20:52
Hossein TwoK's user avatar
\$\endgroup\$
2
  • 1
    \$\begingroup\$If theinfo argument isTrue, then the line printed out is the line on which theCustomError 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 theCustomerError) occurred on the same line, i.e.raise CustomError("user input is empty!", info=True)?\$\endgroup\$CommentedNov 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\$CommentedNov 9 at 21:21

3 Answers3

11
\$\begingroup\$

naming booleans

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

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

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

usePath for a path

        self.file_name = "<unknown file>"

Typing that asstr seems unfortunate; better to make it aPath.

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

Ido appreciate that the constructor always exits with the sameset 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 assignsframe_current = null;to ensure eventual garbage collection.And yes, I understand we don't want spurious referenceskeeping 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 thelinecache.getline() call. Simply elide thedel statements.

spurioustry

Checking whethercurrentframe() gave usNoneis essentially asking "are we running under cPython?",which seems legitimate.But then thetry seems to be trappingAttributeErrorin case.co_name or similar does not exist.That sounds like a "cannot happen" case to me.

Please useexcept AttributeError: if that was the true concern.

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

consistent newline

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


motivation

class PayrollError(Exception):    passclass NoFundsError(PayrollError):    passclass EmployeeIdNotFoundError(PayrollError):    pass

or if it’s something unnecessary or not recommended

Generally, I don't get why an app author wouldwant toraise your CustomError.I mean, apps routinely define an app-specific hierarchysuch as the example I supplied.Once we have seen that calling code can actuallyretry / recover from certain conditions,then defining something more specific than ValueError makes sense.And the tree of error classes conveniently lets a callercatch several things or just one.

But the OP code is mostly concerned withinspecting what's on the call stack.By default the interpreter displays a nice backtrace already.It feels like the code we see herebelongs up in the app's top-level default error handlerwhich 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 explicitlywritten down in """docstrings""" or# comments.

answeredNov 10 at 5:07
J_H's user avatar
\$\endgroup\$
7
\$\begingroup\$

I have a couple reservations here.In the provided example,CustomError is used as a snap-in replacement forValueError. 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 acontext manager or aFinally 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. Iflinecache.getline returns None, then the.strip you are applying could fail and raise an exception. But wait, let's look at thedocs, 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 neverraise an exception — it will return '' on errors (the terminatingnewline character will be included for lines that are found).

I believe the frame object properties egf_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 useself.__class__.__name__ to identify your class, good idea. You can do the same in the__str__ method.

Stack trace

In more complex situations, the fullstack trace is really helpful to have thecontext 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.

answeredNov 10 at 1:32
Kate's user avatar
\$\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 theCustomError 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.

MethodCustomError.__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 tolinecache.getline would be "<unknown file>" and "<unknown func>". There is not much value in that.

You initializeself.file_name to "<unknown file>" andself.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 reasonframe_error = frame_current.f_back raises an exception, then in yourfinally block, you will be attempting to deleteframe_error, which will beNone. So you need to check its value before deleting it. And you should move the initialization statementframe_error = None to a position where it makes more sense, i.e. within theif 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>"
answeredNov 9 at 21:46
Booboo's user avatar
\$\endgroup\$
0

You mustlog 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.