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

Python 2: Forbid type objects as keys. #163

Merged
merged 4 commits into from
Apr 14, 2021
Merged

Python 2: Forbid type objects as keys. #163

merged 4 commits into from
Apr 14, 2021

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Apr 7, 2021

Fixes #153.

Also take the opportunity to clean up the TODOs in _datatypes.py with regards to checking for default comparison: Use a metaclass so we get caching, and only check for the special methods on the type object.

@jamadden jamadden requested a review from icemac April 8, 2021 10:26
@icemac icemac removed their request for review April 9, 2021 06:22
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to remove myself from the reviewers list as I do not understand what's going on here.

src/BTrees/_datatypes.py Outdated Show resolved Hide resolved
@jamadden
Copy link
Member Author

jamadden commented Apr 9, 2021

I have to remove myself from the reviewers list as I do not understand what's going on here.

I apologize for being unclear. I admit to mixing in the fix for #153 ("Python 3 forbids classes as keys, but Python 2 doesn't") with some semi-unrelated cleanup.

The direct fix for #153 was to specifically check type and forbid it as a key, or, if it's a subclass of type (a metaclass) forbid it if the metaclass doesn't implement comparison. This happened by default on Python 3, but I think it's valuable to add for those migrating from Python 2 — there's nothing worse than BTree corruption!

It turns out there are a lot of differences among the supported implementations of Python as to how "doesn't implement comparison" can be detected. The refactoring changes both simplified and improved the speed of that. (I hope.)

Please let me know if I can help clarify further. I'd very much like to get this merged and released.

Fixes #153.

Also take the opportunity to clean up the TODOs in _datatypes.py with regards to checking for default comparison: Use a metaclass so we get caching, and only check for the special methods on the type object.
def __call__(self, item):
self._check_default_comparison(item)
if isinstance(item, _HasDefaultComparison):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_HasDefaultComparison is defined as a new class above but where is item registered for it, so this check can become true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_HasDefaultComparison is defined as a new class above

Right. Specifically, it's defined as a new abstract base class, i.e., something that extends abc.ABCMeta. (We could do this with a regular type subclass, but extending ABCMeta gets us caching of the results of isinstance automatically.)

where is item registered for it, so this check can become true?

That's the beauty part (as the saying goes). By making _HasDefaultComparison implement __subclasshook__, we algorithmically define what isinstance(item, _HasDefaultComparison) means. No need to register anything! The algorithm is the same as what was here before, just moved into __subclasshook__. (The expression isinstance(item, Cls) basically boils down to Cls.__subclasscheck__(type(item)); ABCMeta defines __subclasscheck__ to do some caching, and if the answer is not in the cache, to call Cls.__subclasshook__().)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did that help clear things up sufficiently?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL, I think I now understood how it works. Maybe adding a link to the Python documentation would help future readers of the code who rarely use abstract base classes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@jamadden jamadden merged commit 39af4f1 into master Apr 14, 2021
@jamadden jamadden deleted the issue153 branch April 14, 2021 10:25
@jamadden
Copy link
Member Author

Thank you!

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

Successfully merging this pull request may close these issues.

Python 2: OxBTrees allow types as keys; Python 3 does not
2 participants