-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Connection state #2671
base: main
Are you sure you want to change the base?
Connection state #2671
Conversation
benedikt-bartscher
commented
Feb 20, 2024
•
edited
Loading
edited
I missed that PR before. I kinda like the idea of having a SessionStatus field. Do you think you could later update this PR to use the change from #3388 once they are merged ? |
@Lendemor no worries, it wasn't really finished yet. I will rebase once your PR is merged. |
#3388 PR is merged. Feel free to ping me when this one is ready for review. |
3a0e905
to
f9dc16b
Compare
@benedikt-bartscher Got it, I'll review whenever the PR is marked as ready 👍 |
3a994ec
to
9c3350b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i started looking at the test changes, but i think if we just avoid sending these new fields in the delta then the tests can stay the way they are.
Anything else missing here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hitting a couple of issues with this
- all states seem to share the same _session_status; i think this needs to be declared as a
pydantic.PrivateAttr
so each instance gets its own copy - the
RECONNECTED
state does not seem to "stick". it shows up once when the initial reconnection happens, but then immediately changes toCONNECTED
again, so detecting the reconnected state is not really possible if there is an on_load or some other event processed before the handler that is supposed to check the status.
We might need some integration tests for this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a test case and the above issues addressed (accidentally clicked approve earlier)
Thanks for looking into this!
|
Marking this as draft until ready for review. |