zopefoundation / persistent

automatic persistence for Python objects
https://pypi.org/project/persistent/
Other
46 stars 28 forks source link

Fix issue 124 #129

Closed jamadden closed 4 years ago

jamadden commented 4 years ago

There are three commits showing the iterations I went through.

Rationale to remove usage of the internal functions:

Fixes #124

icemac commented 4 years ago

I like it very much that you tackled this issue although I am not able to review it. Maybe it should be tested on Python 3.9 on TravisCI as well.

icemac commented 4 years ago

There is #123 where I ran the tests on Python 3.9 on Travis CI but they where green without the changes here. According to #124 this is because the pure Python version us used. Can we do something to force using the C version or at least check if it can be used to have a more clear indicator for changes in the C API in future?

jamadden commented 4 years ago

I ran the tests on Python 3.9 on Travis CI but they where green without the changes here. According to #124 this is because the pure Python version us used

In this case, it's because the breaking changes are not actually released yet. They are not in 3.9a3, only in git master. Everything compiles and runs fine (except for that one warning) on 3.9a3 still.

Can we do something to force using the C version

I think so. I like the way zope.interface is handling things (a decorator, plus a distinction between PURE_PYTHON=0 and PURE_PYTHON=1) and I'd like to spread that around.

jamadden commented 4 years ago

have a more clear indicator for changes in the C API in future?

We could also try setting CFLAGS=-Werror. That would have caught the PyEval_CallObject issue, but, since _Py_NewReference and friends are private, it wouldn't have caught that. Plus there's the possibility of false positives unless it's scoped very carefully.

jamadden commented 4 years ago

Because this is such a simple change that only affects specialized builds of Python, I'm tempted to merge it.