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: type RequestException members (not Any) #8989

Merged
merged 6 commits into from
Sep 20, 2023

Conversation

ErikBjare
Copy link
Contributor

@ErikBjare ErikBjare commented Oct 26, 2022

Thought it was weird that mypy didn't catch the following:

import requests

try:
    r = requests.get("http://localhost:666")
except requests.RequestException as e:
    # e.response can be None for some subtypes of RequestException (like ConnectionError, Timeout, etc.)
    # basically for all except HTTPError (I think?)
    # Therefore, it crashes with:
    #   AttributeError: 'NoneType' object has no attribute 'status_code'
    if e.response.status_code == 500:
        pass

Hopefully this is a reasonable change.

@github-actions

This comment has been minimized.

@ErikBjare
Copy link
Contributor Author

ErikBjare commented Oct 26, 2022

Looks like this triggered a bunch of issues in repos, should be possible to make more precise by specifying that HTTPError always has them set (not sure about this assumption, though).

@github-actions

This comment has been minimized.

ErikBjare added a commit to ErikBjare/requests that referenced this pull request Oct 26, 2022
As far as I can tell, this is the only internal construction of `ConnectionError` that doesn't include `request` or `response`.

Part of improving typing for exceptions in requests: python/typeshed#8989
@ErikBjare
Copy link
Contributor Author

If psf/requests#6270 is considered mergeable/correct, that means we can further specify ConnectionError.request to not be None, which would get rid of the final complaints from mypy_primer.

@JelleZijlstra
Copy link
Member

Thanks! Let's wait for resolution on the requests PR so that we can merge this without the mypy-primer regression.

@hauntsaninja hauntsaninja added the status: deferred Issue or PR deferred until some precondition is fixed label Oct 28, 2022
@github-actions

This comment has been minimized.

@intgr
Copy link
Contributor

intgr commented Sep 19, 2023

Let's wait for resolution on the requests PR

Oct 26, 2022

image

@github-actions
Copy link
Contributor

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

schemathesis (https://github.com/schemathesis/schemathesis)
+ src/schemathesis/models.py: note: In member "call" of class "Case":
+ src/schemathesis/models.py:340: error: Argument 2 to "_get_code_message" of "Case" has incompatible type "Optional[Request]"; expected "PreparedRequest"  [arg-type]
+ src/schemathesis/models.py: note: At top level:
+ src/schemathesis/runner/events.py: note: In member "from_exc" of class "InternalError":
+ src/schemathesis/runner/events.py:195: error: Item "None" of "Optional[Request]" has no attribute "url"  [union-attr]

@intgr
Copy link
Contributor

intgr commented Sep 19, 2023

Maybe we can live with the regression and merge this as is?

As long as psf/requests#6270 isn't merged, ConnectionError.request is truly None'able, the additional errors from mypy are correct and may help fix bugs in user code.

@JelleZijlstra
Copy link
Member

Given that it's been almost a year, I agree. I'll wait a day or so in case other typeshed maintainers disagree, then merge this PR.

@JelleZijlstra JelleZijlstra self-assigned this Sep 19, 2023
@JelleZijlstra JelleZijlstra merged commit ddebb83 into python:main Sep 20, 2023
46 checks passed
@ErikBjare ErikBjare deleted the patch-1 branch September 22, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred Issue or PR deferred until some precondition is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants