Let subclasses of BaseAdapterRegistry customize the data structures. #228
Conversation
|
I've added the migration method. It turns out to be useful to fix persistent registries that are suffering from the refcount issue described in #227 as well. The next step would be to update |
|
A question about the three methods I added ( |
Add extensive tests for this. Fixes #224. Also adds test for, and fixes #227 Add BAR.rebuild() to fix the refcount issue, and to change datatypes. It works by using the new methods allRegistrations() and allSubscriptions() to re-create the data in new data structures. This makes fresh calls to subscribe() and register(). I went this way instead of trying to manually walk the data structures and create them because the logic in those methods is fully tested.
|
Are there any interested reviewers? I'm really hoping to be able to release this feature soon as large registries can become a considerable pain point. |
|
I would like to review, but at the moment my time is a bit limited. If you give me 2 more days? |
|
Of course, certainly! That wasn't meant as a threat to merge it unreviewed. Just as a request for reviews, with context. |
| """ | ||
| if existing_leaf_sequence is None: | ||
| return (new_item,) | ||
| return existing_leaf_sequence + (new_item,) |
There was a problem hiding this comment.
I do not know if performance is an issue here, but concatenating tuples generates a new object which might have some impact on performance
There was a problem hiding this comment.
True! But:
(a) that's what the existing code did, so there shouldn't be a regression (other than the slight cost of the added method call); and
(b) even if registering new utilities/adapters is on a critical fast path of an application (seems doubtful to me, but assumptions like that can obviously be wrong!), this is probably the least part of that, when considering actually creating the component, testing whether it needs to be added to the subscribers sequence, emitting the event about the registration, etc.
src/zope/interface/adapter.py
Outdated
| except StopIteration: | ||
| return iter(()) | ||
|
|
||
| return chain((first,), it) |
|
Thanks for the review @ale-rt ! |
Also tweak docs.
Add extensive tests for this. Fixes #224.
Also adds test for, and fixes #227
Based on #226 so that CI runs. The first two commits aren't relevant to this PR.
I'd like to add a migration method to help existing persistent utilities convert to new data structures, but before that's done I'll open this as a draft so the general approach can be looked at.