Skip to content

Fix requests session hooks type #1794

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

Merged
merged 1 commit into from
Jan 26, 2018
Merged

Conversation

garrettheel
Copy link
Contributor

@garrettheel garrettheel commented Dec 19, 2017

Each key in hooks is a list, not a callable directly. Current error when using hooks is:

mypy: Callable[[Response], Any] has no attribute "append" (E)

https://github.com/requests/requests/blob/24092b11d74af0a766d9cc616622f38adb0044b9/requests/hooks.py#L18

@JelleZijlstra
Copy link
Member

I'm slightly worried about this because List is invariant, so this change may make mypy reject adding objects that are a subtype of Callable[[Response], Any]. I'll have to look into it some more.

@JelleZijlstra
Copy link
Member

We need to split the two usages of the _Hooks alias in the current code. The type of the hooks field is correct (the values are lists of callables), but the hooks= argument to .request() actually is a mapping from text to callables, not to list of callables (https://github.com/requests/requests/blob/master/requests/models.py#L230). So you should change the two usages of _Hooks separately.

@garrettheel
Copy link
Contributor Author

@JelleZijlstra it appears that requests actually accepts either a Callable or an Iterable as the param here: https://github.com/requests/requests/blob/master/requests/models.py#L177-L180

I re-worked this a little to take that into account; wanna take another look? Thanks!

@JelleZijlstra JelleZijlstra merged commit 372b541 into python:master Jan 26, 2018
@garrettheel garrettheel deleted the patch-1 branch January 29, 2018 07:15
rhysparry added a commit to rhysparry/typeshed that referenced this pull request Feb 8, 2018
* python/master: (269 commits)
  allow Optional[float] for asyncio.wait() timeout arg (python#1860)
  Clean up the pytype blacklist. (python#1851)
  filter function: make the callable of the first overload non-optional so that mypy is able to select the second overload for the  case. (python#1855)
  Accept Text in distutils *Version classes (python#1854)
  Add attrs library (python#1838)
  Add type stub for the lzma module (python#1844)
  Add ImportError attributes name, path for Python 3.3+ (python#1849)
  Add 'message' to click.decorators.version_option (python#1845)
  PEP 553 and PEP 564 (python#1846)
  Adding py36 additions to datetime module (python#1848)
  Remove incomplete thrift stubs that cause false positives. (python#1827)
  adding curses.ascii, curses.panel and curses.textpad modules (python#1792)
  Fix requests session hooks type (python#1794)
  Add StringIO.name and BytesIO.name (python#1802)
  Fix werkzeug environ type (python#1831)
  Change the return type of unittest.TestCase.fail() to NoReturn (python#1843)
  _curses.tparm works on bytes, not str (python#1828)
  Refine types for distutils.version (python#1835)
  Add stub for stat.filemode (python#1837)
  modify pytype_test to run from within pytype too, and support python3 (python#1817)
  ...
@ikonst
Copy link
Contributor

ikonst commented Feb 3, 2019

@garrettheel this has caused a false negative here:

session.hooks['response'] = _response_hook

A workaround is

session.hooks['response'] = [_response_hook]

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.

3 participants