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

Typing of requests.HTTPError possibly too strict #10764

Closed
moltob opened this issue Sep 25, 2023 · 8 comments
Closed

Typing of requests.HTTPError possibly too strict #10764

moltob opened this issue Sep 25, 2023 · 8 comments
Labels
stubs: false positive Type checkers report false errors

Comments

@moltob
Copy link

moltob commented Sep 25, 2023

Recent contributions #8989 and #10740 improved the typing of requests.HTTPError, so now we have:

class HTTPError(RequestException):
request: Request
response: Response
def __init__(self, *args: object, request: Request, response: Response) -> None: ...

This is causing issues in our code as it appears to be incompatible to the way requests itself is using the type:

Now the request argument is mandatory. However, requests itself does not do it this way, e.g. see the implementation of Response.raise_for_status:

image

Secondly, the type is now restricted to be Request. In particular this prevents assigning a PreparedRequest (which is not derived from Request). However, this is exactly what requests is doing in RequestException.__init__, in addition to explicitly allowing not to pass a request argument:

image

Please reconsider the strict typing.

@JelleZijlstra @srittau

akx added a commit to akx/typeshed that referenced this issue Sep 25, 2023
`requests` itself allows a `None` type for `Response`;
it will internally attempt to read it from the `request` passed.

Fixes python#10764
akx added a commit to akx/typeshed that referenced this issue Sep 25, 2023
`requests` itself allows a `None` type for `Response`;
it will internally attempt to read it from the `request` passed.

There is thus no strict guarantee that a `HTTPError`
will have a truthy `request`.

Fixes python#10764
@akx
Copy link
Contributor

akx commented Sep 25, 2023

It looks like the type of request in the exception should be Request | PreparedRequest, since response.request is typed to be a PreparedRequest; trying to manually do HTTPError(response=resp, request=resp.request) now raises

Argument "request" to "HTTPError" has incompatible type "PreparedRequest"; expected "Request"

@srittau
Copy link
Collaborator

srittau commented Sep 25, 2023

Thanks to @akx's PR, the request type is now relaxed. But per the example given I think the assumptions in #8989 and #10740 are wrong and we need to allow None values (and defaults) again for request and response.

@hsheth2
Copy link
Contributor

hsheth2 commented Sep 25, 2023

I opened #10776 to fix this - @srittau let me know what you think.

@srittau
Copy link
Collaborator

srittau commented Sep 27, 2023

types-requests 2.31.0.6 should fix the remaining issues when it gets released within the next few hours. Please comment here if there are still issues remaining, so we can reopen the issue. Thanks again to @akx and @hsheth2 for the fixes!

@srittau srittau closed this as completed Sep 27, 2023
@djowett
Copy link

djowett commented Oct 11, 2023

I still get an error like this in 2.31.0.6, is that intended?

 error: Missing named argument "response" for "HTTPError"  [call-arg]

the argument is indeed missing, but isn't that valid?

@srittau
Copy link
Collaborator

srittau commented Oct 11, 2023

#10776 made only the request optional. If the response should also be optional, we'd need a PR to be able to judge the impact.

@srittau
Copy link
Collaborator

srittau commented Oct 13, 2023

Some more discussion about HTTPError.response in #10875.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 1, 2023

I still get an error like this in 2.31.0.6, is that intended?

 error: Missing named argument "response" for "HTTPError"  [call-arg]

the argument is indeed missing, but isn't that valid?

This should be fixed in the latest version of types-requests, following #10875.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stubs: false positive Type checkers report false errors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants