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

♻️ Replace _types module by asgiref.typing #1044

Merged
merged 4 commits into from
May 27, 2021
Merged

♻️ Replace _types module by asgiref.typing #1044

merged 4 commits into from
May 27, 2021

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented May 24, 2021

Following what was discussed on #991 about adding asgiref.

This PR only does what the title suggests, and adapts the current code in the easiest way so it doesn't break and pass mypy.

@@ -31,7 +49,7 @@ async def startup(self) -> None:
# Keep a hard reference to prevent garbage collection
# See https://github.com/encode/uvicorn/pull/972

await self.receive_queue.put({"type": "lifespan.startup"})
await self.receive_queue.put(LifespanStartupEvent(type="lifespan.startup"))
Copy link
Member Author

Choose a reason for hiding this comment

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

Not doing this gives me:

uvicorn/lifespan/on.py:52: error: Type of TypedDict is ambiguous, could be any of ("LifespanStartupEvent", "LifespanShutdownEvent")
uvicorn/lifespan/on.py:52: error: Argument 1 to "put" of "Queue" has incompatible type "Dict[str, str]"; expected "Union[LifespanStartupEvent, LifespanShutdownEvent]"

If you have suggestions, I'm open to change it 😗

Copy link
Member

Choose a reason for hiding this comment

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

I think if you do something like

lste : LifespanStartupEvent = {"type": "lifespan.startup"}
await self.receive_queue.put(lste)

it should be ok, though I have not tested it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it should work. But I don't see any real improvement. Is there any?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it should work. But I don't see any real improvement. Is there any?

well not having a mypy error is one in my mind, I'm no mypy pro but found the TypedDict experience quite frustrating and solved most of my issues by doing that, I'm not married to it though so if you have a better idea I'm all ears.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the idea would be the current implementation on the PR, but it's the same to me. I've never done what I propose on this PR tho, I always do what you suggest, so I'm going to change the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -31,7 +49,7 @@ async def startup(self) -> None:
# Keep a hard reference to prevent garbage collection
# See https://github.com/encode/uvicorn/pull/972

await self.receive_queue.put({"type": "lifespan.startup"})
await self.receive_queue.put(LifespanStartupEvent(type="lifespan.startup"))
Copy link
Member

Choose a reason for hiding this comment

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

I think if you do something like

lste : LifespanStartupEvent = {"type": "lifespan.startup"}
await self.receive_queue.put(lste)

it should be ok, though I have not tested it

@euri10 euri10 added the tooling label May 27, 2021
uvicorn/lifespan/on.py Outdated Show resolved Hide resolved
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

thanks !

@Kludex Kludex merged commit 926a8f5 into encode:master May 27, 2021
tony added a commit to eduflow/uvicorn that referenced this pull request Jan 3, 2022
tony added a commit to eduflow/uvicorn that referenced this pull request Jan 3, 2022
tony added a commit to tony/uvicorn that referenced this pull request Jan 8, 2022
tony added a commit to tony/uvicorn that referenced this pull request Jan 8, 2022
tony added a commit to tony/uvicorn that referenced this pull request Feb 11, 2022
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* ♻️ Replace _types module by asgiref.typing

* ♻️ Use explicit hinting instead of TypeDict constructor

* Update uvicorn/lifespan/on.py

Co-authored-by: euri10 <[email protected]>

Co-authored-by: euri10 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants