Move Convert Into A Class (fixes #12)

OpenMath / py-openmath

An OpenMath 2.0 implementation in Python

MIT License

15 stars 4 forks source link

Move Convert Into A Class (fixes #12) #14

Closed tkw1536 closed 6 years ago

tkw1536 commented 6 years ago

Previously, the convert module stored registered conversion functions inside global state. This made for akward code and also made it difficult to register multiple conversion functions for the same symbol.

This PR fixes the issue by introducing a new Converter class which stores all previously global state. For convenience, an instance of this class called DefaultConverter is provided as well. Furthermore, this PR updates the tests and documentation accordingly.

defeo commented 6 years ago

I am ok with the idea, but this is an API change, which would break compatibility (in py-scscp, for example).

The fix is easy, so no big deal, but wouldn't it make sense to expose DefaultConverter.to_python() and DefaultConverter.to_openmath() at the module level?

tkw1536 commented 6 years ago

I thought about the same thing after making the PR. I will see about adding an appropriate commit.

On March 2, 2018 1:13:01 PM GMT+01:00, Luca De Feo notifications@github.com wrote:

I am ok with the idea, but this is an API change, which would break compatibility (in py-scscp, for example).

The fix is easy, so no big deal, but wouldn't it make sense to expose DefaultConverter.to_python() and DefaultConverter.to_openmath() at the module level?

defeo commented 6 years ago

Oh, and the register_ functions too, I suppose. I know Nicolas was using them in his code.

nthiery commented 6 years ago

Looking into this. Should the shorthands to_python -> DefaultConverter.to_python be considered as deprecated? If not, should the test file just use them?

nthiery commented 6 years ago

Done in #15.

nthiery commented 6 years ago

By the way a question: we use the name "Converter" for two distinct things:

A conversion function for a specific type
Instances of the class Converter which manage a collection of such conversions In the documentation, I have started using systematically "conversion" for the former. I haven't changed the name of the variables/parameters though

florian-rabe commented 6 years ago

I was going to merge this, but I don't know what the failed checks mean.

@tkw1536, can you have a look and merge?

tkw1536 commented 6 years ago

I wanted to have a look at this pull request some time ago, but for some reason I must have forgotten. I will have a look again.

tkw1536 commented 6 years ago

I have rebased this onto master and made a few more fixes:

added the register function to the global convert.py namespace
restored the exact tests that are on master, to ensure that we are actually backwards compatible
removed the Python Nightly test, because libxml appears to be broken on it

From my end there is now nothing else to prevent this from being merged.