Refactor subscription implementation #1002
Conversation
|
Thanks for adding the Here's a preview of the changelog: This release fixes the ASGI subscription implementation by handling disconnecting clients properly. Additionally, the ASGI implementation has been internally refactored to match the AIOHTTP implementation. |
|
Uhm, I'll take a look at the failing test tomorrow. |
Codecov Report
@@ Coverage Diff @@
## main #1002 +/- ##
==========================================
+ Coverage 97.53% 97.75% +0.22%
==========================================
Files 77 78 +1
Lines 2755 2805 +50
Branches 385 385
==========================================
+ Hits 2687 2742 +55
+ Misses 42 41 -1
+ Partials 26 22 -4 |
There was a problem hiding this comment.
I've added a couple of comments, will do another review later :)
By the way, @wuyuanyi135 you also worked on this, so if you want a sticker feel free to request one here: https://forms.gle/dmnfQUPoY5gZbVT67
This reverts commit 926ec41.
Co-authored-by: Patrick Arminio <[email protected]>
strawberry/aiohttp/views.py
Outdated
| ) -> None: | ||
| operation_id = message["id"] | ||
| payload: dict = message["payload"] | ||
| payload: StartPayload = message["payload"] # type: ignore |
There was a problem hiding this comment.
@patrick91 mypy didn't know which type from the payload Union to expect here so I added payload: StartPayload. Then mypy complained again:
error: Incompatible types in assignment (expression has type "Union[Dict[str, Any], StartPayload, DataPayload, ErrorPayload]", variable has type "StartPayload")
To deal with this I added # type: ignore. Do you know an alternative solution?
There was a problem hiding this comment.
yes! we can use type guards in future, but for now I'd use cast(StartPayload, message["payload"]) :)
There was a problem hiding this comment.
we can update to use cast or merge now and update to use typeguards in future, I'll let you decide :D
There was a problem hiding this comment.
I just tried using TypeGuards (interesting concept), unfortunately our mypy version doesn't play nice with them: error: Variable "typing_extensions.TypeGuard" is not valid as a type. Temporarely updating mypy solved this issue, but mypy then complaints about various other things (like library stubs not being installed for flask, dateutil and click). I'll probably use TypeGuards and somehow satisfy mypy (probably using type ignore). When we update mypy the type ignore can be removed.
There was a problem hiding this comment.
Forget the last part, I went with cast for now. Using TypeGuards became a little tricky since a functions input is validated to have a certain type but I need the whole message to confirm its payload has a certain type. I'll look at typeguards in more depth in a separate PR.
Description
This PR extends and supersedes #897. The main focus of this PR was to refactor the ASGI subscription implementation and increase its test coverage by porting the AIOHTTP implementations tests. Now both subscription implementations are very similar which makes them easier to maintain and potentially abstract in the future. As a side effect the checks added in #897 have been tested.
Types of Changes
Issues Fixed or Closed by This PR
Checklist