Closed alendit closed 5 years ago
This is incredible to say the least! Thanks for taking the time to play with the library and to send such fun contribution! I have a lil question before merging it. Do you think you could rewrite that test to use another operator instead of the @
so it doesn't cause syntax error for tests on other python versions?
Excited with all the possibilities open by this change \o/
Hey,
Thanks for looking into it so quickly. I'll clean up the code a little in the following days (hopefully tomorrow), make the suggested changes and ping you again.
On Mon, Feb 18, 2019, 18:09 Lincoln de Sousa notifications@github.com wrote:
@clarete commented on this pull request.
In forbiddenfruit/init.py https://github.com/clarete/forbiddenfruit/pull/24#discussion_r257774622:
class PyObject(ctypes.Structure): pass
+def make_opaque_ptr(name):
- newcls = type(name, (ctypes.Structure,), {})
- return ctypes.POINTER(newcls)
+#PyObject_p = ctypes.POINTER(PyObject)
Sorry for the minor nitpick but I'd get rid of this line! :)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/clarete/forbiddenfruit/pull/24#pullrequestreview-204888100, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE83aIEmU0AgN429SKa_Pqdy1yieOgwks5vOt4sgaJpZM4bA67c .
Hi,
so couple of additions: you can now throw NotImplementedError
in the overridden dunders which will make the interpreter proceed with overload resolution. It's useful for type checking the arguments. I also made the overriding revertable.
We'd have to look into supporting older python version, because they likely have a different PyTypeObject
layout. The principle should be the same. With python 2.7 EOL nearing, you could consider dropping the support of the legacy versions in newer releases of your library, but it's another discussion.
Very cool changes! Really liked the possibility to revert the patching of the dunders!
One question though! Do you think there's any possibility to try to get the same functionality of those two methods written in C using the cffi
? Not really a blocker but not having to compile the C code might make it easier to install and play with the library! If there's no good solution for that, let me know if you need any help making the tests work.
To your point about the python versions. That's indeed a good thing to think about. The test suite still tries to use python 2.5 although not even travis supports that anymore! I have no idea about which version we should actually support (besides 3.6+). I'd love to hear what you think!
Yeah, thought of the way to avoid having a native module. It relies on kind of undocumented behaviour of ctypes, but the library's name isn't allowedfruit
...
I also added a bunch of tests and a skip decorator for the older versions.
As for legacy support: I see a real use case for this library in something one-offish, like a Jupyter notebook, where you want to have some nice syntax, but not concerned that much about the stability in many different environments. To my knowledge, nobody runs python <2.7 there, and even 2.7 becomes rarer and rarer.
Used your cool PyDict_SetItem
trick to get the PyNotImplemented
, which makes the code look cleaner.
This is awesome! Thanks a lot for the updates on the patch! They look great! I'm ready to merge them, let me know if you're OK with the merge or if you want to do anything else before that!
On the python compatibility topic, I guess we really don't need 2.5 & 2.6 anymore! I'd keep 2.7 for now. What's your take on which python 3 versions should be supported?
I'd say you can merge it. The test failure is unrelated to the changes, I think. The test tries to overwrite pop
method of the dict
. pop
isn't defined in the __dict__
though. Instread there's a custom struct on the dict
type which holds references to the built-in method implementations. Not sure why the tests succeeds from time to time, though.
Yeah, it makes sense to support 2.7 at least until the EOL. I can look into supporting it for this patch. Most likely the PyTypeObject
layout is slightly different, but it's unlikely that the difference is huge.
As for Python 3, I think everything >=3.3 is a legitimate target.
I'd say you can merge it. The test failure is unrelated to the changes, I think. The test tries to overwrite
pop
method of thedict
.pop
isn't defined in the__dict__
though. Instread there's a custom struct on thedict
type which holds references to the built-in method implementations. Not sure why the tests succeeds from time to time, though.
Thanks a lot for the debugging on this issue! Had no idea it was the case! Always good to learn more about python! :)
Yeah, it makes sense to support 2.7 at least until the EOL. I can look into supporting it for this patch. Most likely the
PyTypeObject
layout is slightly different, but it's unlikely that the difference is huge.
That'd be great! But I feel like it's a good time to merge this PR since as you said, your additions didn't really change the tests (only for the better!!)
As for Python 3, I think everything >=3.3 is a legitimate target.
Nice! Thanks for the comment! I'll get the travis updated! <o/
Related to #11, although right now it doesn't fix that issue exactly. This patch add the overloading of many dunder methods, i.e. "add" or "matmul" for built-in types.
Implementing "str" and "repr" should be easy enough, I'll look into it, if the patch direction is approved.