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

Fix/calllist typing #690

Merged
merged 5 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGES
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
0.24.1
------

* Reintroduced overloads for better typing in `CallList`.
* Added typing to `Call` attributes.


0.24.0
------

Expand Down
17 changes: 15 additions & 2 deletions responses/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import json as json_module
import logging
import socket
from collections import namedtuple
from functools import partialmethod
from functools import wraps
from http import client
Expand All @@ -18,12 +17,14 @@
from typing import Iterator
from typing import List
from typing import Mapping
from typing import NamedTuple
from typing import Optional
from typing import Sequence
from typing import Sized
from typing import Tuple
from typing import Type
from typing import Union
from typing import overload
from warnings import warn

import yaml
Expand Down Expand Up @@ -96,7 +97,11 @@ def __call__(
]


Call = namedtuple("Call", ["request", "response"])
class Call(NamedTuple):
request: "PreparedRequest"
response: "_Body"


_real_send = HTTPAdapter.send
_UNSET = object()

Expand Down Expand Up @@ -241,6 +246,14 @@ def __iter__(self) -> Iterator[Call]:
def __len__(self) -> int:
return len(self._calls)

@overload
def __getitem__(self, idx: int) -> Call:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recent version of mypy doesn't require overload, why do we necessarily need it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mypy==1.6.1, which is the most current version and it still throws:

error: Item "list[Call]" of "Call | list[Call]" has no attribute "request"  [union-attr]

Changes in this PR should fix that. I can't really imagine how would mypy guess the appropriate combination of input and output types without overload.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous CI passed just fine with mypy, although I saw some flakiness in results in the last couple of months

What is the difference then?

Copy link
Contributor Author

@howamith howamith Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI will still pass because mypy isn't being run against the tests. The definition of CallList's __getitem__ method is fine. It's where it's used that becomes a problem. You can reproduce this locally by removing the line that excludes the tests from mypy in the .ini file.

This is why I've suggested running static analysis on the tests.

Copy link
Collaborator

@beliaev-maksim beliaev-maksim Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then we indeed need to enabling mypy on tests to validate this PR and all the future PRs

Copy link
Contributor Author

@howamith howamith Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth getting this out to fix the regression, and then dealing with enabling static analysis in the tests in a separate PR? My original intention was to do that here but after removing the exclusion from the mypy config file there were 754 errors. Fixing those is going to make the diff really noisy and feels like a separate issue (IMO).

Happy to address here if that's preferred!

...

@overload
def __getitem__(self, idx: slice) -> List[Call]:
...

def __getitem__(self, idx: Union[int, slice]) -> Union[Call, List[Call]]:
return self._calls[idx]

Expand Down
33 changes: 33 additions & 0 deletions responses/tests/test_responses.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from io import BufferedReader
from io import BytesIO
from typing import Any
from typing import List
from typing import Optional
from unittest.mock import Mock
from unittest.mock import patch
Expand All @@ -20,6 +21,7 @@

import responses
from responses import BaseResponse
from responses import Call
from responses import CallbackResponse
from responses import PassthroughResponse
from responses import Response
Expand Down Expand Up @@ -2035,6 +2037,37 @@ def run():
assert_reset()


def test_response_calls_indexing_and_slicing():
@responses.activate
def run():
responses.add(responses.GET, "http://www.example.com")
responses.add(responses.GET, "http://www.example.com/1")
responses.add(responses.GET, "http://www.example.com/2")

requests.get("http://www.example.com")
requests.get("http://www.example.com/1")
requests.get("http://www.example.com/2")
requests.get("http://www.example.com/1")
requests.get("http://www.example.com")

# Use of a type hints here ensures mypy knows the difference between index and slice.
individual_call: Call = responses.calls[0]
call_slice: List[Call] = responses.calls[1:-1]

assert individual_call.request.url == "http://www.example.com"

assert call_slice == [
responses.calls[1],
responses.calls[2],
responses.calls[3],
]
assert [c.request.url for c in call_slice] == [
"http://www.example.com/1",
"http://www.example.com/2",
"http://www.example.com/1",
]


def test_response_calls_and_registry_calls_are_equal():
@responses.activate
def run():
Expand Down