Bytes vs text handling - Githubissues

betamaxpy / betamax

A VCR imitation designed only for python-requests.

https://betamax.readthedocs.io/en/latest/

Other

565 stars 62 forks source link

Bytes vs text handling #123

Closed wimglenn closed 6 years ago

wimglenn commented 7 years ago

Had to write a new serializer to address the issue ... also changes in serializer bases were needed.

https://github.com/betamaxpy/betamax/issues/122

sigmavirus24 commented 7 years ago

Hi @wimglenn I haven't had a chance.

Further, is there any way to reproduce the "issue" you're seeing? Why does it require adding a PickleSerializer to the core when we have a different package for alternative serializers?

wimglenn commented 7 years ago

Yes, I've already described clearly how to reproduce it in issue 122 - then you asked me to prepare a PR so here it is.

I did actually notice the package for alternate serializers, but I was not able to achieve the fix for this issue without changes to the core.

wimglenn commented 7 years ago

@sigmavirus24 The change to the core was necessary because the core code explicitly opened file handles in text mode (done by SerializerProxy class), rather than leaving it up to the serializer class to specify the mode. Since python's pickle writes a binary stream, you can't use the file opened in text mode.

If you would look at the changes, the way I've implemented it allows the Serializer class to specify the mode, but it's also backwards compatible in that omitting to specify it will default to the old behaviour (open in text mode).

wimglenn commented 7 years ago

It would be quite fine to put the new serializer class to the other package instead (a MessagePack serializer might be nice too), but the change in the core would still be needed. And your JSONSerializer will continue to be misbehaving, I couldn't find any way to fix that one.

hroncok commented 6 years ago

Hi @wimglenn. I see what you did and the code looks reasonable. Here are my thoughts:

the change in core that allows the serializer to pick if reading and writing in binary or text mode is good
whether the PickleSerializer belongs here or to the alternative serializers package I don't know, but would say the latter, as it is only specifically needed if you are juggling with bytes in headers
this definitively needs a docs entry explaining the problem and solution. if we go with adding this to the extra serializers package, I'd suggest to add it here http://betamax.readthedocs.io/en/latest/third_party_packages.html

Sorry for the late reply. Are you still interested in this? If so, we can work together to make it happen. Let me know.

wimglenn commented 6 years ago

Hi @hroncok I agree the serializer should move into the other package, but since the issue required changes in core, there would be no way to write the test code in such a way that it demonstrated the failing test case and the fix.

Feel free to dust off this PR and chop it up or move the code around. I don't really have anything more to offer on this.