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

Avoid surprising mutation and remove Any from the kwargs type. #6

Merged
merged 3 commits into from
Sep 19, 2024
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
25 changes: 10 additions & 15 deletions src/ape_pie/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,13 @@
from requests import Response, Session

from .exceptions import InvalidURLError
from .typing import ConfigAdapter
from .typing import ConfigAdapter, RequestKwargs

if TYPE_CHECKING:
# TODO Use typing.Self once we drop support for Py310
from typing_extensions import Self


sentinel = object()


def is_base_url(url: str | furl) -> bool:
"""
Check if a URL is not a relative path/URL.
Expand Down Expand Up @@ -75,7 +72,7 @@ class APIClient(Session):
def __init__(
self,
base_url: str,
request_kwargs: dict[str, Any] | None = None,
request_kwargs: RequestKwargs | None = None,
**kwargs: Any, # subclasses may require additional configuration
):
# base class does not take any kwargs
Expand All @@ -85,16 +82,14 @@ def __init__(

self.base_url = base_url

# set the attributes that requests.Session supports directly, but only if an
# actual value was provided.
for attr in self.__attrs__:
val = request_kwargs.pop(attr, sentinel)
if val is sentinel:
continue
setattr(self, attr, val)

# store the remainder so we can inject it in the ``request`` method.
self._request_kwargs = request_kwargs
self._request_kwargs = {}
# set the attributes that requests.Session supports directly
for k, v in request_kwargs.items():
if k in self.__attrs__:
setattr(self, k, v)
else:
# store the remainder so we can inject it in the ``request`` method.
self._request_kwargs[k] = v
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose not to annotate self._request_kwargs as RequestKwargs, because mypy only allows assignment in it with literals. The fact that k came from the same type...

PyRight either is smarter or doesn't seem to mind.

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with switching to pyright in CI instead of mypy if that makes life better


def __enter__(self):
self._in_context_manager = True
Expand Down
41 changes: 39 additions & 2 deletions src/ape_pie/typing.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,41 @@
from typing import Any, Protocol
from __future__ import annotations

from http.cookiejar import CookieJar
from typing import TYPE_CHECKING, Protocol, TypedDict

if TYPE_CHECKING:
# imports from stubs
from _typeshed import Incomplete
from requests.api import _HeadersMapping
from requests.sessions import (
_Auth,
_Cert,
_Data,
_Files,
_HooksInput,
_Params,
_TextMapping,
_Timeout,
_Verify,
)


class RequestKwargs(TypedDict, total=False):
# Supposed to be ParamSpec.kwargs of `requests.request`
params: _Params
data: _Data
headers: _HeadersMapping
cookies: CookieJar | _TextMapping | None
files: _Files | None
auth: _Auth | None
timeout: _Timeout | None
allow_redirects: bool
proxies: _TextMapping | None
hooks: _HooksInput | None
stream: bool | None
verify: _Verify | None
cert: _Cert | None
json: Incomplete


class ConfigAdapter(Protocol):
Expand All @@ -8,7 +45,7 @@ def get_client_base_url(self) -> str: # pragma: no cover
"""
...

def get_client_session_kwargs(self) -> dict[str, Any]: # pragma: no cover
def get_client_session_kwargs(self) -> RequestKwargs: # pragma: no cover
"""
Return kwargs to feed to :class:`requests.Session` when using the client.

Expand Down
24 changes: 23 additions & 1 deletion tests/test_client_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from requests.auth import HTTPBasicAuth

from ape_pie import APIClient
from ape_pie.typing import RequestKwargs


@pytest.mark.parametrize("attr", Session.__attrs__)
Expand Down Expand Up @@ -32,9 +33,30 @@ def test_can_override_defaults(attr, value):
assert attr in Session.__attrs__
vanilla_session = Session()

client = APIClient("https://example.com", {attr: value})
client = APIClient("https://example.com", {attr: value}) # type: ignore[assignment]

vanilla_value = getattr(vanilla_session, attr)
client_value = getattr(client, attr)
assert client_value != vanilla_value
assert client_value == value


def test_config_does_not_mutate():
imported_settings: RequestKwargs = {
"auth": ("djkhaled", "youplayedyourself"),
}

class MyConfig:
@staticmethod
def get_client_base_url():
return "https://example.com"

@staticmethod
def get_client_session_kwargs():
return imported_settings

one = APIClient.configure_from(MyConfig)
another_one = APIClient.configure_from(MyConfig)

assert one.auth == ("djkhaled", "youplayedyourself")
assert another_one.auth == ("djkhaled", "youplayedyourself")
Copy link
Contributor Author

@CharString CharString Sep 18, 2024

Choose a reason for hiding this comment

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

After reading #3... Does this actually work? Or is this one of the surprises Victorien meant?

Copy link
Member

Choose a reason for hiding this comment

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

it's a different thing. A surprising one indeed 😬

3 changes: 2 additions & 1 deletion tests/test_config_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

from ape_pie import APIClient
from ape_pie.exceptions import InvalidURLError
from ape_pie.typing import RequestKwargs


class TestConfigAdapter:
Expand All @@ -17,7 +18,7 @@ def get_client_base_url():
return "https://from-factory.example.com"

@staticmethod
def get_client_session_kwargs():
def get_client_session_kwargs() -> RequestKwargs:
return {
"verify": False,
"timeout": 20,
Expand Down
4 changes: 2 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ commands = mypy src
[testenv:isort]
extras = dev
skipsdist = True
commands = isort --check-only --diff .
commands = isort --check-only --diff src tests

[testenv:black]
extras = dev
Expand All @@ -39,7 +39,7 @@ commands = black --check src tests docs
[testenv:flake8]
extras = dev
skipsdist = True
commands = flake8 .
commands = flake8 src tests

[testenv:docs]
basepython=python
Expand Down
Loading