-
Notifications
You must be signed in to change notification settings - Fork 42
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
WebSocket support #23
WebSocket support #23
Conversation
This pull request introduces 13 alerts when merging 141709b into de8e0f0 - view on LGTM.com new alerts:
|
This pull request introduces 13 alerts when merging 553158d into de8e0f0 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
==========================================
+ Coverage 99.07% 99.21% +0.13%
==========================================
Files 5 7 +2
Lines 326 512 +186
Branches 48 55 +7
==========================================
+ Hits 323 508 +185
Misses 1 1
- Partials 2 3 +1
Continue to review full report at Codecov.
|
This pull request introduces 9 alerts when merging aa58b85 into 0de4f23 - view on LGTM.com new alerts:
|
This pull request introduces 9 alerts when merging 807dfc1 into 0de4f23 - view on LGTM.com new alerts:
|
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.
Left one or two silly comments, overall looks really great. Automat based state machine looks very nice!
client_protocol, | ||
iosim.makeFakeClient(client_protocol), | ||
) | ||
return pump, client_protocol |
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 don't know anything about iosim
, so I only skimmed this part of the code and did not go deeply into iosim
itself.
partner_connection_lost, | ||
enter=done, | ||
outputs=[], | ||
) |
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.
This whole state machine logic is very beautiful and so clear to read!
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.
Looks great!
This pull request introduces 9 alerts when merging 6bd063a into 0de4f23 - view on LGTM.com new alerts:
|
This builds on the new
iosim.IOPump
-based tests to add WebSocket support with proper tests.Needs:
new_protocol_ws
implementations; unify