Skip to content

Conversation

@pcmoritz
Copy link
Contributor

No description provided.


def put(self, object value, ObjectID object_id=None):
def put(self, object value, ObjectID object_id=None,
serialization_context=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could you the type to the new parameter?

Copy link
Member

Choose a reason for hiding this comment

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

I believe it can be any Python object having the required methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is true!

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

Can you add a unit test?


def put(self, object value, ObjectID object_id=None):
def put(self, object value, ObjectID object_id=None,
serialization_context=None):
Copy link
Member

Choose a reason for hiding this comment

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

I believe it can be any Python object having the required methods

@pcmoritz
Copy link
Contributor Author

Good catch, I'm adding the tests!

@pcmoritz pcmoritz force-pushed the plasma-serialization-context branch from 63ff3cd to 1a4fade Compare September 1, 2017 04:08
@pcmoritz
Copy link
Contributor Author

pcmoritz commented Sep 1, 2017

+1 This should now be ready to merge

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1

@asfgit asfgit closed this in c6295f3 Sep 1, 2017
@wesm wesm deleted the plasma-serialization-context branch September 1, 2017 12:59
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