Skip to content
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

Refactor net gevent #18

Closed
wants to merge 1 commit into from
Closed

Refactor net gevent #18

wants to merge 1 commit into from

Conversation

gabor-boros
Copy link
Member

Description

The net_gevent contains lot of style issues and duplicates some net classes. This PR adjusts the code base to be able to use the duplicated classes' ancestor from net. Also fixing some styling issues.

Please note that Connection class in net_gevent moved up from the bottom, and the SocketWrapper removed in favour of net'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 SocketWrapper for net_gevent.

@gabor-boros gabor-boros added enhancement New feature or request quality issue fix labels Aug 5, 2018
@gabor-boros gabor-boros added this to the Package release milestone Aug 5, 2018
@gabor-boros gabor-boros self-assigned this Aug 5, 2018
Copy link
Contributor

@asakatida asakatida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



class GeventCursorEmpty(ReqlCursorEmpty, StopIteration):
pass


class Connection(net.Connection):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would still need the old init where it provides the instance class.

@gabor-boros
Copy link
Member Author

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.

@asakatida
Copy link
Contributor

Sounds good. I will try to carve out some time today to get some more work done on my open issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants