-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Python: Allow replacement scans on pyrelations, move to BoxRenderer and use pending query API for relations #5962
Conversation
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.
Some nitpicks 😅
Thanks @Mause Co-authored-by: Elliana May <me@mause.me>
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.
Looks good to me, great piece of functionality and seems like this was relatively low hanging fruit.
That being said it was invisible to me haha
I had one final comment regarding the removal of the GIL unlock to print the progress bar.
Sadly we dont/cant have Jupyter tests, I'm curious if this might break something there
if (!success) { | ||
return GetError() + "\n"; | ||
} | ||
if (!collection) { |
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.
Do we not want to throw an exception here instead?
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.
In general results should not throw exceptions because they might be used in places where those are unexpected.
This PR makes several improvements to the Python Relational API:
str
representation of relation objects