Skip to content

Conversation

@mrocklin
Copy link
Member

This makes msgpack decode arrays as tuples rather than lists, allowing them to be used within containers like set and dict that expect hashable elements.

cc @mariusvniekerk

I took a quick look at this but then got side tracked by other things. I thought I'd push it up here in case you were interested in carrying on.

@rbubley
Copy link
Contributor

rbubley commented May 23, 2018

It seems that the unit tests in msgpack-python use a different approach to the issue of tuples.

See, e.g. lbolla/msgpack-python@c8042ea

@mariusvniekerk
Copy link
Collaborator

mariusvniekerk commented May 24, 2018

I like the approach in that and we can implement set, tuple, frozenset which is probably good enough for now? Implemented it in #2003

@mrocklin
Copy link
Member Author

mrocklin commented May 24, 2018 via email

@mariusvniekerk
Copy link
Collaborator

I've fixed some of the failing tests here in a branch of mine. Whats the easiest way to add those commits

@mrocklin
Copy link
Member Author

mrocklin commented May 30, 2018 via email

@mrocklin mrocklin changed the title [WIP] Use tuples in msgpack Use tuples in msgpack Jun 11, 2018
@mrocklin
Copy link
Member Author

OK, hypothetically this passes tests. There are some things that concern me though:

  1. We are inconsistent about our use of tuples and lists. We use tuples everywhere except if the tuple contained a Serilized object, in that case we use a list
  2. We actually have to do this, because we only traverse through lists to find serialized objects.

I would not be surprised if this came back to bite us later.

@mrocklin
Copy link
Member Author

Well, lets see what happens. Merging.

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