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

Simple JSON override interface #4825

Merged
merged 8 commits into from Nov 4, 2015
Merged

Simple JSON override interface #4825

merged 8 commits into from Nov 4, 2015

Conversation

asakatida
Copy link
Contributor

This depends on #4818

@danielmewes
Copy link
Member

You're awesome @grandquista ! :)

(mentioning @Tryneus to put this on your screen. I'm going to move #4818 and this one into the 2.2 milestone)

@@ -401,12 +403,14 @@ def _get_ssl_context(self, ca_certs):


class ConnectionInstance(object):
def __init__(self, parent):
def __init__(self, parent, json_encoder=None, json_decoder=None):
Copy link
Member

Choose a reason for hiding this comment

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

The json_encoder and json_decoder code appears to be repeated verbatim for each type of connection. This code should probably be put straight into the Connection class, which is used as the base class for the different types - DefaultConnection, net_twisted.Connection, net_tornado.Connection, and net_asyncio.Connection.

Since this code is common to all the connection types (and doesn't really rely on any ConnectionInstance variables), Connection should just pull json_encoder and json_decorder out of the kwargs, and provide the _get_json_decoder and _get_json_encoder functions that the child classes will inherit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the solution to use Connection properties and reduce code.

@Tryneus
Copy link
Member

Tryneus commented Oct 28, 2015

@grandquista - looks good, but I had a comment to try and reduce the code duplication. Thanks for submitting this!

@Tryneus
Copy link
Member

Tryneus commented Nov 4, 2015

Awesome, merging. Thanks again for the help, @grandquista!

Tryneus added a commit that referenced this pull request Nov 4, 2015
@Tryneus Tryneus merged commit 02d4e6f into rethinkdb:next Nov 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants