Skip to content
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

Split packer into a class that supports msgpack's ExtType #70

Merged
merged 4 commits into from
Dec 5, 2014

Conversation

fahhem
Copy link
Contributor

@fahhem fahhem commented Dec 4, 2014

There is, however, one drawback. datetime objects sent over the wire aren't wire-compatible. Fixing that makes the code very complicated and I don't know how needed that is since v0.1.0 is going to be backwards-incompatible anyway.

@ticosax
Copy link
Owner

ticosax commented Dec 4, 2014

Hi, thanks for this contribution.

If I understand correctly, this changeset try to extend support of datetime.* objects by using pickle as Serializer before passing them to msgpack ?

I'm OK to break backward compatibility for this release, but there is a quick win you can apply to not break too much things by keeping pseud.common.msgpack_packb and pseud.common.msgpack_unpackb

pseud/common.py

p = Packer()
msgpack_packb = p.packb
msgpack_unpackb = p.unpackb

then you can reduce the size of the diff drastically. And since each Packer instance holds a caching dictionary, using it as a singleton will improve overall performance.
What do you think ?

@fahhem
Copy link
Contributor Author

fahhem commented Dec 5, 2014

This changeset is to allow users of pseud to extend the packer's translation table to include custom classes. I want to 'send' some classes in my project and have msgpack turn it into an ExtType and back on the other side.

I don't want a singleton for this, as it's not necessary at all. If you want a singleton for performance, you can pass in a singleton packer instance you made into your clients and servers. Without the singleton enforced, it allows one to create asymmetric serialization (which is something I use): class A sent from the client becomes class B on the server. While this is possible with the singleton if you're either the client or server, it won't work if you're both (testing).

@ticosax
Copy link
Owner

ticosax commented Dec 5, 2014

I understand better now, thanks for explanation.
so if you fix the tests. I think it will be good to go.

@fahhem fahhem force-pushed the master branch 3 times, most recently from 153cdd3 to 7432f4e Compare December 5, 2014 12:37
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.98%) when pulling 7432f4e on fahhem:master into b52af25 on ezeep:master.

@fahhem
Copy link
Contributor Author

fahhem commented Dec 5, 2014

Oh, I couldn't run the tests locally using tox so I thought they were normally broken. If you're using travis, I think you should remove tox.ini to not confuse developers.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.98%) when pulling 69c3ed0 on fahhem:master into b52af25 on ezeep:master.

@ticosax
Copy link
Owner

ticosax commented Dec 5, 2014

I plan to use tox within Travis, to make everyone happy :)

@fahhem fahhem force-pushed the master branch 2 times, most recently from 78caba5 to cf059f2 Compare December 5, 2014 14:20
@coveralls
Copy link

Coverage Status

Coverage increased (+0.19%) when pulling cf059f2 on fahhem:master into b52af25 on ezeep:master.

@fahhem
Copy link
Contributor Author

fahhem commented Dec 5, 2014

tox doesn't support the custom zeromq version stuff, as far as I could tell. In any case, I finally got the tests working (there was a lot of flakiness, is that normal?)

@ticosax
Copy link
Owner

ticosax commented Dec 5, 2014

That's a great contribution,
Thank you.
I'll create follow up tickets for the issues you just raised

ticosax added a commit that referenced this pull request Dec 5, 2014
Split packer into a class that supports msgpack's ExtType
@ticosax ticosax merged commit ca13b80 into ticosax:master Dec 5, 2014
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.

None yet

3 participants