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

Switch from Sentinel types to Enums #154

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pgjones
Copy link
Member

@pgjones pgjones commented Aug 25, 2022

The latter are much easier to work with when type hinting and can be
used successfully with mypyc, whereas the former are sadly very
difficult in both aspects.

This loses the nice property of type(NEED_DATA) is NEED_DATA (as
expanded on in the deleted docs section). However, I don't think this
is widely used in practice.

Closes #153. See also #8 for reasoning behind the introduction of the type property.

@njsmith what do you think about this?

@pgjones pgjones force-pushed the master branch 3 times, most recently from c42389e to df6649d Compare August 25, 2022 11:02
The latter are much easier to work with when type hinting and can be
used successfully with mypyc, whereas the former are sadly very
difficult in both aspects.

This loses the nice property of `type(NEED_DATA) is NEED_DATA` (as
expanded on in the deleted docs section). However, I don't think this
is widely used in practice.
@tomchristie
Copy link
Contributor

This looks to me like a breaking API change that would impact Uvicorn and HTTPX.

I do prefer the API, tho.

@Kludex
Copy link
Contributor

Kludex commented Sep 1, 2022

I can check this on Uvicorn tonight. jfyk

@tomchristie
Copy link
Contributor

@Kludex Sure thing. Permalinks to relevant parts of code in the comment above. (Tho I can see that's not obvious.)

@Kludex
Copy link
Contributor

Kludex commented Oct 30, 2022

This was merged on httpcore: encode/httpcore#579

@Kludex
Copy link
Contributor

Kludex commented Oct 30, 2022

The only problem on uvicorn was:

            event = h11.InformationalResponse(
                status_code=100, headers=[], reason="Continue"
            )

Issue:

uvicorn/protocols/http/h11_impl.py:537: error: Argument "headers" to "InformationalResponse" has incompatible type "List[<nothing>]"; expected "Union[Headers, List[Tuple[bytes, bytes]], List[Tuple[str, str]]]"  [arg-type]
uvicorn/protocols/http/h11_impl.py:537: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
uvicorn/protocols/http/h11_impl.py:537: note: Consider using "Sequence" instead, which is covariant
Found 1 error in 1 file (checked 53 source files)

I've used this branch on uvicorn to check the changes I'd need: https://github.com/encode/uvicorn/pull/1744/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants