-
-
Notifications
You must be signed in to change notification settings - Fork 857
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
Should HTTPError be a base class for RequestError and HTTPStatusError? #1108
Comments
Would need to include all the top-level exceptions from #1095, so: HTTPError = (HTTPStatusError, RequestError, NotRedirectResponse, CookieConflict, StreamError) |
That's a really interesting idea, thanks! An alternative might be to keep it as an exception class, as a base class of |
I'd support this option. |
Actually, yeah, an exception class is probably better, you could probably do something to print out a deprecation warning that way? |
I guess theres a question here, re: if we should actually prefer to have that as a base class permanently, or if we'd only be doing this to capture both cases during a gentle deprecation. Incidentally I'm not sure there is any way we'd be able to raise a deprecation warning to users importing and using an exception class in an |
Actually I think there's a way: Module # httpx/__init__.py
def __getattr__(name: str): # type: ignore # Called if `httpx.<name>` was not found
if name == "HTTPError":
from ._utils import warn_deprecated
# (Not sure about this error message, considering we might want to point at "it could be any of the top-level exceptions, pick one explicitly".)
warn_deprecated("HTTPError is deprecated, please use one of RequestError or HTTPStatusError instead")
from ._exceptions import HTTPError
return HTTPError
raise AttributeError(f"module {__name__} has no attribute {name}") Result: $ python -Wo -c "import httpx; print(httpx.HTTPError)"
/Users/florimond/Developer/python-projects/httpx/httpx/__init__.py:45: DeprecationWarning: HTTPError is deprecated
warn_deprecated("HTTPError is deprecated")
<class 'httpx._exceptions.RequestError'> |
I think we should only keep |
My only goal here was to highlight a relatively innocent change that would wind up being a horribly breaking change. To be fair, pre-1.0 breaking changes are completely fair game, but it's nice to at least alert users to the breaking changes! That being said, I do like the concept of "one base exception per library", but in httpx's case I think that having a few base exceptions is reasonable. For instance, some of the code I was updating when I realized this might be a problem looks a lot better than the original code with only |
I'm going through some of my projects preparing them for the 0.14 release and it occurred to me that the following line may be confusing and trip folks up.
httpx/httpx/_exceptions.py
Line 282 in 9409900
This will break something like the following since the HTTPStatusError is now no longer caught, it'd just raise straight up and crash the program.
Instead, can we change the line to:
That way at least something close to (or maybe exactly like?) the original functionality is maintained, at least until it is formally deprecated?
The text was updated successfully, but these errors were encountered: