Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider the advice on exception types #9

Open
amontalenti opened this issue Jan 4, 2016 · 8 comments
Open

Reconsider the advice on exception types #9

amontalenti opened this issue Jan 4, 2016 · 8 comments

Comments

@amontalenti
Copy link
Owner

I noticed that people found the "rarely create exception types" suggestion in this style guide problematic, for some very valid reasons.

There is the worry that by raising built-in types, like ValueError and LookupError, you could mask actual coding errors. It was suggested that it's always better to subclass these types and raise your own exception type, to make the difference clear. This was in a discussion with @ramalho and @dabeaz on Twitter.

Quite a few people brought up that the impulse for this suggestion is that a caller shouldn't need to know about 10 different exception types to catch every last well-defined error condition. It seems most agree on that. @jackdied said: "Most exceptions I see are defined once, raised once and caught once." This would indeed be bad style. Many suggested that when making many different types, you could simplify life for the caller by having a "root" error type for a given library/API, and making all special errors subclass this.

I took a look at how requests does this. They introduce 19 exception types related to HTTP and a few of them subclass built-in exception types. Most of them also sub-class a "root" type, called RequestException, which seems to cover library-specific errors you may choose to intentionally ignore, like InvalidURL.

https://github.com/kennethreitz/requests/blob/master/requests/exceptions.py

I doubt that most user code should be creating exception trees like this -- HTTP can fail in a number of specific and well-known ways, and this is a library of standard library quality. It's also important to point out that even requests chooses to keep most of these types implementation-free.

So, I wonder if I'm too harsh on exception types since Python does make it exceedingly easy to create no-impl exception classes that are instantly useful in explaining well-known errors and disambiguate them from code problems. I'll reconsider this one.

@lucaswiman
Copy link

Using very generic exceptions types (especially ValueError) can make code that does different things in different error cases very difficult. In the guide you say:

A good rule of thumb for whether you should create your own exception type is to figure out whether a caller should catch it every time they call your function. If so, you probably should make your own type.

I think a better rubric would be:

A good rule of thumb for whether you should create your own exception type is to figure out whether a caller should ever catch it when they call your function. If so, you probably should make your own type, preferring to subclass the relevant built in exception types.

@amontalenti
Copy link
Owner Author

@lucaswiman I think you have a good point there. Thank you for the suggestion. I think this section will definitely get some rework soon.

@amontalenti
Copy link
Owner Author

Some interesting thoughts on exceptions in Python from the Launchpad team here:

https://dev.launchpad.net/ExceptionGuidelines

@jackdied
Copy link

Apologies for not posting a response yet. I'll try to do more soon. But in general my real advice is closer to Hettinger's from his "Beyond PEP-8" talk. Yes, use them to disambiguate. But what I see in practice and rail against in code reviews is custom exception spam.

@chepner
Copy link

chepner commented May 14, 2020

I think this is bad advice. Python 3 added many subclasses of OSError (FileExistsError, FileNotFoundError, etc.) precisely to avoid the need to examine an error after catching it to determine whether you should have caught it. (For example, in Python 2, you might catch an OSError, ignore it if it is for FileExistsError, or re-raise it otherwise. In Python 3, you simply catch FileExistsError and leave the other ones uncaught.)

@amontalenti
Copy link
Owner Author

@chepner Thank you for this comment -- I didn't know about this stdlib change in Python 3 re: OSError, it's a very good indication that specialized exception types serve a useful purpose. I think I've been well-swayed that the advice on exception types in this style guide may not be for the best, so I'm going to try revising this section.

@dabeaz
Copy link

dabeaz commented May 14, 2020

My general advice on exceptions is two-fold. First, resist the urge to catch exceptions except when absolutely necessary---it is often better to just let the code crash or for errors to be caught at a higher level. Second, if you're going to intentionally raise an exception, use a custom exception. The latter often helps separate cases of "I meant to do that" (custom exception) versus "there's a programming error in my code" (built-in exception).

@dabeaz
Copy link

dabeaz commented May 14, 2020

I'd add there is a social element to it as well. I've been on too many projects where people won't read the freaking traceback message--instead they just copy/paste it into the issue tracker with some vague "code's broken" message. I really don't want to be someone's personal debugger. If I see one my custom exceptions at the bottom, I can just look at it and pretty much know what they did "Oh, you're getting THAT error because you did X." On the other hand, if I see a Python built-in exception at the bottom, well, then maybe there's a bug in my code. I know that because I would never intentionally raise a built-in exception.

@amontalenti amontalenti self-assigned this May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants