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

[requests] Improve exception class constructors #10740

Merged

Conversation

intgr
Copy link
Contributor

@intgr intgr commented Sep 20, 2023

@intgr intgr force-pushed the requests-improve-exception-constructors branch from eff7274 to 56db738 Compare September 20, 2023 12:39
@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Should we make the arguments also optional on HTTPError?

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

cloud-init (https://github.com/canonical/cloud-init)
+ tests/unittests/sources/azure/test_errors.py:169: error: Missing named argument "request" for "HTTPError"  [call-arg]
+ tests/unittests/sources/azure/test_errors.py:169: error: Missing named argument "response" for "HTTPError"  [call-arg]
+ tests/unittests/sources/azure/test_imds.py:64: error: Missing named argument "request" for "HTTPError"  [call-arg]
+ tests/unittests/sources/test_azure.py:343: error: Missing named argument "request" for "HTTPError"  [call-arg]

paasta (https://github.com/yelp/paasta)
+ paasta_tools/mesos_maintenance.py:213: error: Missing named argument "request" for "HTTPError"  [call-arg]
+ paasta_tools/mesos_maintenance.py:213: error: Missing named argument "response" for "HTTPError"  [call-arg]

apprise (https://github.com/caronc/apprise)
+ test/helpers/rest.py:63: error: Missing named argument "request" for "HTTPError"  [call-arg]
+ test/helpers/rest.py:63: error: Missing named argument "response" for "HTTPError"  [call-arg]
+ test/test_attach_http.py:59: error: Missing named argument "request" for "HTTPError"  [call-arg]
+ test/test_attach_http.py:59: error: Missing named argument "response" for "HTTPError"  [call-arg]
+ test/test_config_http.py:54: error: Missing named argument "request" for "HTTPError"  [call-arg]
+ test/test_config_http.py:54: error: Missing named argument "response" for "HTTPError"  [call-arg]

@srittau
Copy link
Collaborator

srittau commented Sep 20, 2023

Should we make the arguments also optional on HTTPError?

Why add HTTPError.__init__() at all? It's just inherited in the implementation.

@intgr
Copy link
Contributor Author

intgr commented Sep 20, 2023

Why add HTTPError.__init__() at all? It's just inherited in the implementation.

#8989 created an assumption that request/response attrs are always present and not None in HTTPError (they're optional in other error classes). That was done specifically to reduce regressions in mypy_primer (because users had already made this assumption): #8989 (comment)

class HTTPError(RequestException):
    request: Request    # <-- can't be None
    response: Response  # <-- can't be None

That assumption is true for errors originating from requests library proper, but as can be seen here, does not hold for third party code raising these exceptions.

Relaxing HTTPError.__init__ isn't correct, but maybe it's a useful tradeoff between introducing regressions and accuracy. I'll leave it up to you, maybe you have a better gut feeling for making such tradeoffs.

@srittau
Copy link
Collaborator

srittau commented Sep 20, 2023

It's a tradeoff. The primer output is a bit concerning, although most of the problems seem to be in test code. I'm fine with this change, despite the primer output. Otherwise we should revert the | None annotations in HTTPError, which is probably worse.

@JelleZijlstra JelleZijlstra merged commit 71d81f8 into python:main Sep 23, 2023
47 checks passed
@intgr intgr deleted the requests-improve-exception-constructors branch October 1, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants