aio-libs / aiohttp

Asynchronous HTTP client/server framework for asyncio and Python
https://docs.aiohttp.org
Other
15.14k stars 2.02k forks source link

Consider making _RequestContextManager part of the public API #7247

Open smheidrich opened 1 year ago

smheidrich commented 1 year ago

Is your feature request related to a problem?

The problem is that _RequestContextManager is marked as private via its underscore prefix despite being the return value of methods like ClientSession.get(), ClientSession.post(), etc.

I understand you mainly intend for people to use these methods as async context managers in which case this type shouldn't be of any concern, but you also made it awaitable on its own and used it like that in many places in the documentation, so clearly there is some interface to it beyond its usage as context manager which is considered public.

Having this be a private type causes issues e.g. when writing wrappers around ClientSessions that just pass the objects returned by ClientSession.get() etc. along: As it is now, one has to either annotate the return type of the wrapper method as _RequestContextManager, which is formally wrong, or write a custom type stub for something that has the same signature as _RequestContextManager, which can be used as the return type (necessitating an extra typing.cast() inside the method).

Describe the solution you'd like

I would like it if _RequestContextManager became RequestContextManager and was thereby considered part of aiohttp's public API.

Maybe the name could be changed because it's not just a context manager (which in a sense is my whole point - if it was, people could just consider its type AbstractAsyncContextManager[ClientResponse] and call it a day).

Describe alternatives you've considered

  1. Leave things as they are and have people who write wrappers come up with their own workarounds.
  2. Wait for someone to write a 3rd party package that introduces a workaround, e.g. custom type stubs and instructions how to make them override aiohttp's type hints or wrappers around aiohttp that do nothing but change these types.

Related component

Client

Additional context

Faintly related: #21 (confusion that might have been averted if this type was public)

Code of Conduct

MtkN1 commented 5 months ago

Here is a workaround:

from collections.abc import Awaitable
from contextlib import AbstractAsyncContextManager
from typing import Protocol, assert_type

import aiohttp

class ResponseContextManagerProtocol(
    Awaitable[aiohttp.ClientResponse],
    AbstractAsyncContextManager[aiohttp.ClientResponse],
    Protocol,
): ...

class WrappedClient:
    def __init__(self):
        self._session = aiohttp.ClientSession()

    def get(self, *args, **kwargs) -> ResponseContextManagerProtocol:
        return self._session.get(*args, **kwargs)

    async def __aenter__(self) -> "WrappedClient":
        return self

    async def __aexit__(self, *args, **kwargs) -> None:
        await self._session.__aexit__(*args, **kwargs)

async def main() -> None:
    async with WrappedClient() as client:
        async with client.get("http://example.com") as response_with_context:
            assert_type(response_with_context, aiohttp.ClientResponse)

        response_with_await = await client.get("http://example.com")
        assert_type(response_with_await, aiohttp.ClientResponse)

Define a simple protocol class (ResponseContextManagerProtocol in this code) that mimics the behavior of _RequestContextManager and type annotate the wrapper methods.

$ mypy main.py 
Success: no issues found in 1 source file
Dreamsorcerer commented 2 months ago

This wrapper may be causing issues, see: https://github.com/python/cpython/issues/106429#issuecomment-1623520278

So, we won't be exposing it at this time, as it may need to be reworked or removed. Once that happens, we'll revisit and see what types need to be exposed.