Opened 7 years ago
Closed 7 years ago
#29443 closed Cleanup/optimization (wontfix)
Unify registries in Django
Reported by: | Johannes Maron | Owned by: | |
---|---|---|---|
Component: | Utilities | Version: | dev |
Severity: | Normal | Keywords: | plugin, registry, pattern, utils |
Cc: | info@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Django utilizes registries in various places thought the code, such as the admin, models, template-tags and the check framework.
At the moment all those places provide their own implementation. Considering that this is a recurring pattern I would suggest to unify the implementation. More specifically to provide a common super class and override or extend behavior if need be.
I would also suggest to add this to django.utils
including public documentation, because this pattern could be useful for Django projects and 3rd party libraries as it is to the Django project itself.
Luckily I have written one before. I don't know if it is any good or even compatible implementations in Django but I added a lot of documentation and research. I am happy to share this here:
class Registry: """ A registry to store and retrieve elements. This is an implementation of the "registry pattern" by Martin Fowler (ISBN: 0321127420). A registry can be used interact with a list of objects, without knowing them. It allows a package to define a common interface and to interact with its implementations without holding a reference to them. A typical use case are plugins where a package provides a feature and allows other packages to extend it. In this case the super package might need to interact with its extensions, without knowing them. This concept is used in places like Django's admin or template tags. Typing a registry can help to define a common interface, that needs to be implemented by all extensions. Usage example: ``mainapp/feature.py``:: from common.utils import Registry class BasePlugin: def get_url(self): raise NotImplementedError() plugins = Registry(entry_type=BasePlugin) class MyFeature: def get_plugin_urls(self): return (plugin.get_url() for plugin in plugins) ``featureapp/feature.py``:: from mainapp.feature import BasePlugin class MyPlugin(BasePlugin): def get_url(self): return "https://example.com" ``featureapp/app.py``:: from django.apps import AppConfig from mainapp.feature import plugins class FeatureApp(AppConfig): name = 'featureapp' def ready(self): plugins.register(MyPlugin) In order for a plugin to be registered, the ``register`` call needs to be executed. A good place to do that in Django apps is in the app's `AppConfig.ready` method. Args: entry_type (``type``): The registry will only allow subclasses or instances of this type to be registered. unique (bool): Whether or not elements can be registered multiple times. Default: ``True`` """ def __init__(self, entry_type=None, unique=True): if entry_type is not None and not isinstance(entry_type, type): raise TypeError('"entry_type" expects a class, but got an instance.') self.entry_type = entry_type self.unique = unique self._register = [] def __iter__(self): # The list is copied, to avoid mutation of the registry while iterating. return iter(list(self._register)) def __len__(self): return len(self._register) def clear(self): """Clear all entries from the registry.""" self._register.clear() def register(self, entry): """ Store entry in the registry. Args: entry: Entry that is to be added to the registry. Raises: TypeError: If the entry type does not match the type of the registry instance. ValueError: If the entry is already registered and the registry instance is set to be unique. """ entry_type = entry if isinstance(entry, type) else type(entry) if self.entry_type and not ( isinstance(entry, self.entry_type) or issubclass(entry_type, self.entry_type) ): raise TypeError('"entry" expects %s, but got %s' % (self.entry_type, entry)) if self.unique and entry in self._register: raise ValueError('"%s" is already registered.' % entry) self._register.append(entry) def unregister(self, entry): """ Remove all occurrences of ``entry`` from registry. Args: entry: Entry that is to be removed from the registry. Raises: ValueError: If entry is not registered. """ if entry not in self._register: raise ValueError('"%s" is not registered.' % entry) # If the registry is not unique, an element could be in the list more than once. while entry in self._register: self._register.remove(entry)
Change History (12)
comment:1 by , 7 years ago
Description: | modified (diff) |
---|
comment:2 by , 7 years ago
Description: | modified (diff) |
---|
comment:3 by , 7 years ago
comment:4 by , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 7 years ago
Rather than reinventing the wheel, can we use stuff like https://github.com/pytest-dev/pluggy ?
comment:7 by , 7 years ago
Hi Florian,
thanks for bringing this up. I didn't check 3rd party.
IMHO this particular implementation does too much. I goes way beyond a simple registry. Replacing the existing code will be more difficult and could introduce unwanted bug.
What concerns me more is adding a dependency.
If you prefer using this package, I am happy to run it by the mailing list. I personally would avoid to use it.
Best
-Joe
comment:8 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:9 by , 7 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
This ticket requires more knowledge about the codebase (which I don't have yet). But I would suggest using exceptions like AlreadyRegistered and NotRegistered in AdminSite.
For those interested in taking a look at places implementing registers, here a list of some classes:
- Library: template/library
- Apps: apps/registry.py
- CheckRegistry: core/checks/registry.py
- AdminSite: contrib/admin/sites.py
comment:10 by , 7 years ago
I took a look to the Django own registry implementations, and struggled to find much code that could be factorized in a common class. The structure of the registries themselves are various: list, set, mapping, sometimes several of them. Then the implementations differ much depending on the internal registry structure.
Johannes, could you explore that a bit and show us some example of current code that would benefit from a utility class?
comment:11 by , 7 years ago
Hi Claude,
yes you are absolutely right. Ana and I discovered the same problem during the springs at DjangoConEu'18.
The implementations are extremely different. Maybe a good reason to refactor it, maybe a good reason to just leave it be.
It's definitely a very difficult tasks. Kudos to whomever solves this ;)
Best
-Joe
comment:12 by , 7 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
At first sight, I don't see a compelling reason to refactor the various registry implementations, as they serve different purposes.
I'll close the ticket, but if someone comes later with some POC code that demonstrates advantages to unify registry implementations, we'll happily reconsider that decision.
I also have tests for the example, but it's written in PyTest but it's a start: