search

OpenMath / py-openmath

An OpenMath 2.0 implementation in Python
MIT License
15 stars 4 forks source link

Convert pickle #19

Closed nthiery closed 6 years ago

tkw1536 commented 6 years ago

This doesn't seem to work in Python 3 (at all), hence the tests are completely failing. I have looked at it, but this code seems to be a big huge hack into the internals of python pickle.

nthiery commented 6 years ago

This doesn't seem to work in Python 3 (at all), hence the tests are completely failing. I have looked at it, but this code seems to be a big huge hack into the internals of python pickle.

Hack? Well, it's using the most public available API :-)

But yes, the implementation of pickle has indeed changed with Python3. The same approach will work with Python3, but will take a bit of adaptation. I focused on Python 2 for now, since Sage's support for Python3 is only experimental at this stage.

tkw1536 commented 6 years ago

Hack? Well, it's using the most public available API :-)

Point taken. When I looked at the code and the documentation I did not see Unpickler.dispatch, hence I assumed it was an internal api.

But yes, the implementation of pickle has indeed changed with Python3. The same approach will work with Python3, but will take a bit of adaptation. I focused on Python 2 for now, since Sage's support for Python3 is only experimental at this stage.

I have added support for Python3 and pushed it. The new code now almost works (see below) for me on Python2 and Python3. I have not tested this with Sage though.

There is still one issue that I am unsure of. Could you explain why a string is encoded as OMBytes(bytes='coucou', id=None)? From an OpenMath standpoint a string should also be an OMString (coincidentally, this is also what is produced by the same code in Python3).

tkw1536 commented 6 years ago

Since I did not receive a response, and want to merge this PR, I have added 55f974432c870130ba067d8926cf50ef7617f360, which ensures that strings are converted to OMString.

nthiery commented 6 years ago

I have added support for Python3 and pushed it. The new code now almost works (see below) for me on Python2 and Python3. I have not tested this with Sage though.

Thanks for the Python3 support!

There is still one issue that I am unsure of. Could you explain why a string is encoded as OMBytes(bytes='coucou', id=None)? From an OpenMath standpoint a string should also be an OMString (coincidentally, this is also what is produced by the same code in Python3).

This was just the default behavior of py-openmath, which made plain Python2 strings into OMBytes and unicode strings into OMStrings. Using OMStrings always should be fine, especially with migrating to Python3 in mind.

I pushed a fix to a Sage doctest; I meant to put it in a branch, but it ended up accidently in master; oh well, minor enough.

Cheers,