Fix not implemented error#204
Fix not implemented error#204gabor-boros merged 3 commits intorethinkdb:masterfrom Inveracity:fix-NotImplementedError
Conversation
|
For first review this seems good. Let me check it tomorrow |
gabor-boros
left a comment
There was a problem hiding this comment.
I like that this is a bit more explicit. Also it is easier to see what’s happening. @lsabi would you like to have a second look on this PR? If not I’d merge this and cherry-pick to the python 2-3 branch with the other merged PR
|
I would capture the possibility that a library name passed as parameter is not valid. Basically, I would move the first if statement down and add a second check. `if library is None or self.connection_type is None: self.connection_type = self.net.DefaultConnection` Better safe than sorry |
|
Yep, that’s true tho |
gabor-boros
left a comment
There was a problem hiding this comment.
@Inveracity Could you please adjust the code accordingly? 😇🙏
|
Issues
======
+ Solved 1
Complexity increasing per file
==============================
- rethinkdb/__init__.py 5
See the complete overview on Codacy |
|
Thanks for the review! I hope I made the correct change requested by lsabi 😄 |
|
Looks good to me |
|
LGTM too. I'll check the changes with some more tests and merge this PR 😊 |
|
Thank you so much! |
|
@gabor-boros, could you please cut a release with this included? |
|
@McSinyx Sure, I'll cut a new release today. |
|
Thanks! |
Reason for the change
closes #201
Description
Dynamically importing using the
implibrary causes setuptools and/or pyinstaller to not find the async libraries.I also could not get the pyinstaller --hidden-import "rethinkdb.net_asyncio" to work, probably for the same reason.
With these changes the behavior is exactly the same, except now the async libraries are recognised as packages and pyinstaller can create a binary that works.
I appreciate that this change makes the code less dynamic whenever a new async library gets implemented. Maybe there's a better fix? Or maybe pyinstaller will eventually support your way of structuring the rethinkdb package, but this definitely made it work for me.
Tested with
Checklist