-
Notifications
You must be signed in to change notification settings - Fork 40
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 issues with pickling & unpickling the Table
object
#871
Conversation
for more information, see https://pre-commit.ci
python-sdk/tests/sql/test_table.py
Outdated
def test_if_table_object_can_be_pickled(): | ||
"""Verify if we can pickle Table object""" | ||
pickle.loads(pickle.dumps(Table(name="test"))) |
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.
Let's add this test for File object. The reason self.__dict__
is not available for File
is because it uses slots
.
A class whose instances have no object.dict attribute and define their attributes in a object.slots attribute instead. In attrs, they are created by passing slots=True to @attr.s (and are on by default in attrs.define()/attrs.mutable()/attrs.frozen()).
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.
Got it
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.
But atleast this test will verify that you can pickle and unpickle the object.
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.
Would the following be a better test?
table = Table(name="test")
assert pickle.loads(pickle.dumps(table)) == table
cc @ashb
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.
@kaxil Ya this seems better.
Table
object
Table
objectTable
object
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.
Approving so you are not blocked but please address the review comments
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Description
What is the current behavior?
Currently, Python-sdk is falling with pickling error.
What is the new behavior?
To fix the above issue we need to add getstate() to the table class
Does this introduce a breaking change?
Nope
Checklist