Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type name varies in the repr() depending on the presence of the C extension #92

Closed
jamadden opened this issue Sep 13, 2018 · 3 comments
Closed

Comments

@jamadden
Copy link
Member

jamadden commented Sep 13, 2018

This manifests in breakage of BTrees tests when the C extensions are used (they pass when the pure-Python extensions are available).

This is a somewhat common problem. There's a python-dev thread and bpo-issue about this. In particular, in the issue, and one of the PRs that fixes it it's pointed out that getting the correct qualified name in a C type can be complex depending on things like Py_TPFLAGS_HEAPTYPE (and that's just on Python 3, I don't know if there's an equivalent of ht_qualname on Python 2).

Is this a bug here that we should fix, or should downstream consumers be prepared to deal with the difference? In one of my own downstream projects I dealt with the difference. But BTrees already deals with the difference in its own code to produce equivalent reprs in Python as we used to get from C. (See zopefoundation/BTrees#94)

jamadden added a commit to zopefoundation/BTrees that referenced this issue Sep 14, 2018
Stop being quite so strict to account for the differences between the
C and Python implementations. But do test that we don't break the new
features when we didn't already have a better form.

Fixes #94

Refs zopefoundation/persistent#92
@jimfulton
Copy link
Member

Is there any reason not to fix the instance reprs to avoid this issue?

@jamadden
Copy link
Member Author

Is there any reason not to fix the instance reprs to avoid this issue?

I'm not quite sure what you mean. If you're asking, "can we make a change to cPersistence.Persistent to report the qualified name", my only answer is "it can be complicated" as discussed in the python-dev thread. Right now it's doing the simplest possible thing (largely because I don't consider myself a C API expert so I kept it simple). If you're asking "can we make a change to persistentce.Persistent to use the simple name", sure, we could, but we lose the module name, which is often useful. (That's a change I could implement.)

I'm not sure how big an issue this really is. BTrees was simple to fix. Still, it's an observable difference.

@jimfulton
Copy link
Member

IDK :) Perhaps we expose the logical module name (e.g. BTrees.IOBTree.BTree) rather than the actual one.

IDK how big an issue it is either.

jamadden added a commit that referenced this issue Oct 19, 2018
Fix the repr of the persistent objects to include the module name when
using the C extension. This matches the pure-Python behaviour and the
behaviour prior to 4.4.0.

Fixes #92
clrpackages referenced this issue in clearlinux-pkgs/persistent Oct 22, 2018
…n 4.4.3

Jason Madden (10):
      Back to development: 4.4.3
      Include the module in the C repr
      Clear the error if the C extension proceeds without a __module__ or __name__.
      Always format 8-byte oids as ints in hexadecimal.
      Add a test, and fix the C implementation, for farmatting a 64-bit oid.
      Attempt to fix Windows 2.7's lack of stdint.h
      Exclude the 'terryfy' directory from wheels and sdists
      Merge pull request #98 from zopefoundation/issue97
      Merge pull request #100 from zopefoundation/issue99
      Preparing release 4.4.3

4.4.3 (2018-10-22)
------------------

- Fix the repr of the persistent objects to include the module name
  when using the C extension. This matches the pure-Python behaviour
  and the behaviour prior to 4.4.0. See `issue 92
  <https://github.com/zopefoundation/persistent/issues/92>`_.

- Change the repr of persistent objects to format the OID as in
  integer in hexadecimal notation if it is an 8-byte byte string, as
  ZODB does. This eliminates some issues in doctests. See `issue 95
  <https://github.com/zopefoundation/persistent/pull/95>`_.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants