Conversation
asakatida
left a comment
asakatida
left a comment
There was a problem hiding this comment.
I'm hesitant to make this large fix without a test in place. We can filter through issues and skip this until the test suite refactoring is started.
| import struct | ||
|
|
||
| import gevent | ||
| import gevent.socket as socket |
There was a problem hiding this comment.
This is the reason I noted waiting on tests below. The documentation for this class says it will only block one greenlet and that is essential to being async. I would rather wait for tests. Otherwise socket wrapper should work like connection where it operates on an internal instance object which can be overridden in the subclass constructor. Long term this pattern is a good place for metaclass factories.
| pass | ||
|
|
||
|
|
||
| class Connection(net.Connection): |
There was a problem hiding this comment.
This would still need the old init where it provides the instance class.
|
I see you point. Then what if I mark this PR as blocked until the tests are in place? FYI, the integration test functionality is done with automatic droplet creation. I’ll open a PR tomorrow. |
|
Sounds good. I will try to carve out some time today to get some more work done on my open issues. |
2 participants
Description
The
net_geventcontains lot of style issues and duplicates somenetclasses. This PR adjusts the code base to be able to use the duplicated classes' ancestor fromnet. Also fixing some styling issues.Please note that
Connectionclass innet_geventmoved up from the bottom, and theSocketWrapperremoved in favour ofnet's own one. (The difference was the timeout/deadline parameter)Important
Please be careful with the review and check if I made any mistake. As far as I can see, no breaking stuff introduced with removing
SocketWrapperfornet_gevent.