zopefoundation / Acquisition

Acquisition is a mechanism that allows objects to obtain attributes from the containment hierarchy they're in.
Other
12 stars 12 forks source link

PURE_PYTHON fixes #49

Closed d-maurer closed 3 years ago

d-maurer commented 3 years ago

make Acquired a string (as required by Zope) respect base object's __eq__ fix attribute access for wrapped values (#48)

d-maurer commented 3 years ago

Commit 040aefa fixes #51. However, I am not sure that the fix is optimal.

Unlike Python, Acquisition looks up (most) special methods not via the (wrapper) type but via the (wrapper) instance. The main effect is that those special methods can be acquired (as their name starts with _, that acquisition is never implicit; it must be specially called for). However, when the Python interpreter calls special methods implicitly, it explicitly looks them up via the type. Formerly, the type and instance based special method implementations have been quite different. With the above commit, the type based special method implementation mostly delegates to the instance based implementation (the type based implementation does in addition things usually done by the interpreter and to this end sometimes uses different exceptions than the instance based implementation). I am not sure whether the "C" implementation behaves in the same way. An alternative fix would have been to implement special methods based on the base object's type (as I did with __cmp__). This would prevent those methods to be acquired.

d-maurer commented 3 years ago

@icemac Do you want to reexamine the PR? Since your review I have changed the implementation for (most) "special methods" in a way which might not be optimal.

icemac commented 3 years ago

@d-maurer Currently the tests for the pure Python version fail with a recursion error. – It is great to have CI running. Thank you for adding it to this repository.

d-maurer commented 3 years ago

Michael Howitz wrote at 2021-7-14 00:13 -0700: @.*** Currently the tests for the pure Python version fail with a recursion error. – It is great to have CI running. Thank you for adding it to this repository.

The tests are now successful (for Python 2.7, too).

However, during the fix, I noticed that the step attribute of slice objects is not tested. In the code, I have treated step in analogy to start and stop (mapping None to 1). As there is no test, it is possible under Python 2, that a wrapped slice access sees a different step value than the corresponding bare slice access (1 instead of None). This should not make a big difference. However, we might nevertheless want to extend the tests and ensure that step has the same default value for wrapped and bare item access.

icemac commented 3 years ago

@d-maurer Do you think it is time for a new release or are there other issues which should be solved first?

d-maurer commented 3 years ago

Michael Howitz wrote at 2021-7-15 23:13 -0700: @.*** Do you think it is time for a new release or are there other issues which should be solved first?

I have filed 2 further issues: #48 and #50. Not sure, how important they are.

d-maurer commented 3 years ago

Dieter Maurer wrote at 2021-7-16 08:39 +0200:

Michael Howitz wrote at 2021-7-15 23:13 -0700: @.*** Do you think it is time for a new release or are there other issues which should be solved first?

I have filed 2 further issues: #48 and #50. Not sure, how important they are.

48 is fixed by #49.

Initially, #49 tried to fix #50, too -- for the Python version. However, I reverted this part because it had introduced different behavior between the "C" and the Python version.

I feel that not respecting the rich comparison relations of the base object is a bug, at least it is very different from how acquisition wrapped objects behave otherwise. Not sure, however, whether existing code depends on the current implementation.

icemac commented 3 years ago

I just released https://pypi.org/project/Acquisition/4.8/#files. The wheels should appear in some minutes.