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

Private imports from httpx. #2673

Open
tomchristie opened this issue Aug 27, 2024 · 7 comments · May be fixed by #2709
Open

Private imports from httpx. #2673

tomchristie opened this issue Aug 27, 2024 · 7 comments · May be fixed by #2709
Assignees

Comments

@tomchristie
Copy link
Member

tomchristie commented Aug 27, 2024

Prompted by encode/httpx#3287


starlette is currently using private imports from httpx...

url: httpx._types.URLTypes,
*,
content: httpx._types.RequestContent | None = None,
data: _RequestData | None = None,
files: httpx._types.RequestFiles | None = None,
json: typing.Any = None,
params: httpx._types.QueryParamTypes | None = None,
headers: httpx._types.HeaderTypes | None = None,
cookies: httpx._types.CookieTypes | None = None,
auth: httpx._types.AuthTypes
| httpx._client.UseClientDefault = httpx._client.USE_CLIENT_DEFAULT,
follow_redirects: bool | None = None,
allow_redirects: bool | None = None,
timeout: httpx._types.TimeoutTypes

These are broken against the latest minor point release, which includes this (internal) refactoring... encode/httpx#3245


It's possible that the path of least resistance for the short term is for httpx to re-include URLTypes, tho really we need to be making sure that downstream packages are not using private imports.

Some prior discussion of this also at #2534.

@tomchristie
Copy link
Member Author

This comment makes a good case that this is an httpx issue... encode/httpx#3176 (comment)

@Kludex
Copy link
Member

Kludex commented Sep 1, 2024

@tomchristie , what do you suggest here?

@Kludex
Copy link
Member

Kludex commented Sep 1, 2024

Maybe we can remove httpx as a dependency for the TestClient? It's always been confusing to install httpx for the testclient module for Starlette users. What do you think? Would that be bad for httpx?

@tomchristie
Copy link
Member Author

Maybe we can remove httpx as a dependency for the TestClient?

Depends on exactly what you mean. It'd be an API breaking change, since it currently returns httpx types.

Something I do like about the existing test client is that it's mirroring using an actual Python client library to interact with the app.

One option could be a similar approach as eg. https://www.starlette.io/database/ - Don't have a dependency, but do document how to use httpx as a test client. (We'd need to work through any functional additions that Starlette's TestClient provides. WebSockets anyone?)

Would that be bad for httpx?

We should aim for streamlined. (Consistent project guidelines, docs, tooling approaches etc.)
We shouldn't aim for preferential treatment.

@Kludex
Copy link
Member

Kludex commented Sep 21, 2024

Let's go back to the original issue. What should Starlette do? Should httpx make types publich?

@tomchristie
Copy link
Member Author

Really I'd like to tighten up the type definitions throughout, so that they're not needed...

Eg... aiming for this type of thing...

    def request(
        self,
        method: str,
        url: URL | str,
        *,
        content: typing.Any = None,
        data: typing.Any = None,
        files: typing.Any = None,
        json: typing.Any = None,
        params: QueryParams | Mapping[str, str] | None = None,
        headers: Headers | Mapping[str, str] | None = None,
        cookies: Cookies | CookieJar | Mapping[str, str] | None = None,
        auth: Auth | None = None,
        follow_redirects: bool | None = None,
        allow_redirects: bool | None = None,
        timeout: Timeout | float | None = None,
        extensions: dict[str, typing.Any] | None = None,
    )

@Kludex
Copy link
Member

Kludex commented Sep 29, 2024

What about the USE_CLIENT_DEFAULT constant?

Also, I don't like that data, content, files and json have Any.

@Kludex Kludex linked a pull request Sep 29, 2024 that will close this issue
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 a pull request may close this issue.

2 participants