Skip to content

Conversation

@pashazz
Copy link
Contributor

@pashazz pashazz commented Jan 24, 2019

Previously r.set_loop_type used a global variable to determine loop type, thus making running asynchronous and synchronous code alongside each other impossible.

Now you can do:

r = rethinkdb.RethinkDB()

To set loop type:

r.set_loop_type('asyncio')

To unset loop type:

r.set_loop_type(None)

This is breaking change: here you should do r.set_loop_type for every RethinkDB object used in asynchronous code. This is probably OK since you'd never use the same RethinkDB object in both synchronous and asynchronous code

from rethinkdb.handshake import HandshakeV1_0
from rethinkdb.logger import default_logger

__all__ = ['connect', 'set_loop_type', 'Connection', 'Cursor', 'DEFAULT_PORT']

Choose a reason for hiding this comment

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

This is abusing __all__ because of the setattr happening in RethinkDB init. Otherwise this is going to override the connect method defined in the class on instance instantiations. Without connect here Python rules implicitly say it's a private method, then we expect it above. I'm not a fan of the confusion in all that. Renaming to make_connection or changing it to a class would solve this.

@asakatida
Copy link

@gabor-boros the Codacy issues were all there before, just moved files. I say we just ignore that here. I would like a second set of eyes on my two code comments.

@pashazz
Copy link
Contributor Author

pashazz commented Jan 26, 2019

so everything's ok then?

Copy link
Member

@gabor-boros gabor-boros left a comment

Choose a reason for hiding this comment

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

@grandquista Hell. Unfortunately, I resolved the import related comment but I totally agree, it should be at the top. And I also agree with the renaming what you mentioned.

@gabor-boros
Copy link
Member

@grandquista Can we merge this? What do you think?

@asakatida
Copy link

@gabor-boros I rolled these changes with my comments in #86 so merge that one and then close this

Sent with GitHawk

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.

3 participants