Conversation
|
📝 Docs preview for commit 0786806 at: https://5f4cf7560889c62bd66d666c--fastapi.netlify.app |
|
📝 Docs preview for commit 26cab4e at: https://5f4d139bd2823d541cd7a6c2--fastapi.netlify.app |
Kludex
left a comment
There was a problem hiding this comment.
Do you mind explaining what you're trying to achieve? 🎉
| class ConnectionManager: | ||
| def __init__(self): | ||
| self.active_connections: List[WebSocket] = [] | ||
| self.active_connections: MutableSet[_IdEq(WebSocket)] = set() |
There was a problem hiding this comment.
| self.active_connections: MutableSet[_IdEq(WebSocket)] = set() | |
| self.active_connections: MutableSet[_IdEq[WebSocket]] = set() |
There was a problem hiding this comment.
I've suggested this, but you probably want to create a variable for it. Because on the add you want to use that new variable to something like: _newIdEq(websocket)
There was a problem hiding this comment.
Might be better to drop the __eq__ from HTTPConnecttion
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1991 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 239 239
Lines 7079 7079
=========================================
Hits 7079 7079 ☔ View full report in Codecov by Sentry. |
if you get a disconnect/connect during broadcast you can skip over sockets in the broadcast queue: |
|
📝 Docs preview for commit 465fd03 at: https://5f4d4b65e676710091ba6af8--fastapi.netlify.app |
Co-authored-by: Marcelo Trylesinski <[email protected]>
|
📝 Docs preview for commit 563c486 at: https://5f4d75a1479f1112f7a7d3c6--fastapi.netlify.app |
|
Starlette has a fix for this in 0.16.0 and so this PR needs to change |
|
So, should anything else be done here @graingert or can we close it? If you think there's something that still needs to be done here, I feel the code change and complexity is a bit daunting, I would try to find a way to make it as simple as possible, and easy to read for newcomers. And for example, I wouldn't use attrs but Pydantic, as everything is using Pydantic here, and FastAPI doesn't depend on attrs directly. |
|
The tutorial just needs to be updated to use a set instead |
|
As this won't be handled in this PR, I'm gonna go ahead and close it to clean up things. Thanks! ☕ |
if you get a connect or disconnect during broadcast