Plasma
Plasma copied to clipboard
[Proposal] Refactor pfPython API classes
Using PyCXX (http://cxx.sourceforge.net/PyCXX-Python2.html) we can expose a Python API with much simpler, cleaner, C++-style code.
As an example, pyStream.h
, pyStream.cpp
, and pyStreamGlue.cpp
would be reduced to just ptStream.h
and ptStream.cpp
and would look something like this: https://gist.github.com/dpogue/8502541
This proposal is to rewrite the existing Python API classes in that style and move them to an API subdirectory of pfPython.
As the classes are refactored, the exposed API should be audited and necessary additions/cleanups should be made. The option of adding attributes to make the API more Pythonic is also possible.
This ticket is to guage interest and hear any feedback/concern before actually trying to tackle the refactor.
This seems like a good idea and does appear cleaner. Before judgment, I think it might be useful to know how one of the more complicated interfaces would transfer, not just hsStream, just to be sure of the feasibility.
Also, will this help to address any existing issues or long-standing problems? This would be a big refactor, and not a crucial one AFAIK, so it would be excellent if there was more at stake than code simplification (which is still a useful goal).
I think this sounds/looks pretty cool. The question is whether, like Deledrius said, this is really worth all of the work in the long run. The refactor should happen for the whole API, not just parts, and we'd need someone with the time to get it done.
Okay, I've been looking at this a bit more, and there are a few things that stick out for me as annoyances.
- PyCXX doesn't have its own API for saying that a class has a base type. It's possible to do it using the normal C Python API and pulling a pointer from PyCXX (which could be easily wrapped in a #define), but it's worth mentioning.
- Getters/Setters for properties need to be implemented with getattr/setattr and strings. I don't know how much of a problem this is, since most Plasma stuff is using functions, but I'll play around a bit to see how bad it is.
- No PyCXX API for adding classes or constants to a module. You can do it by assigning named values to a dictionary object, but there's no 1 function call to register something. This is also easily fixed with a #define.
- Still need to do a bunch of plString conversion.
If we patch PyCXX, we can easily support 1 and 3, and probably 2, and contribute those patches upstream. On the other hand, if we don't mind having an in-tree customized PyCXX, we could change it to use plStrings directly and fix 4 as well.
Forking and vendoring is the path to madness
Forking and vendoring is the path to madness
I'm partly inclined to agree. I definitely think the first 3 issues should be fixed upstream. I'll make patches and try to figure out how to SourceForge.
Another thing I realized that might be an issue is the licence. It's BSD licenced, but has some additional clauses: https://sourceforge.net/p/cxx/code/HEAD/tree/trunk/CXX/COPYRIGHT Supposedly it's OSI compatible though.