-
-
Notifications
You must be signed in to change notification settings - Fork 32
Fix not implemented error #204
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
Fix not implemented error #204
Conversation
For first review this seems good. Let me check it tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
imp
library 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