feat: simpler component configuration #279
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR refactors component plugin configuration by introducing a resolver for plugin classes that accepts both dotted paths and plugin names, simplifies allowed_plugin_types initialization, enforces unique plugin name registration, standardizes generated plugin module assignment, and updates documentation accordingly. Sequence Diagram: Plugin Class Resolution via get_plugin_class during SetupsequenceDiagram
participant "System (during setup)" as System
participant "settings"
participant "setup()"
participant "get_plugin_class()" as get_plugin_class
participant "importlib"
participant "plugin_pool"
System->>setup(): Call setup()
activate setup()
setup()->>settings: Reads CMS_COMPONENT_PLUGINS
loop for each entry in CMS_COMPONENT_PLUGINS
setup()->>get_plugin_class: get_plugin_class(entry)
activate get_plugin_class
alt entry is dotted path (e.g., "my_module.MyClass")
get_plugin_class->>importlib: import_module("my_module")
activate importlib
importlib-->>get_plugin_class: module
deactivate importlib
get_plugin_class->>importlib: getattr(module, "MyClass")
activate importlib
importlib-->>get_plugin_class: PluginClass
deactivate importlib
else entry is plugin name (e.g., "MyPluginName")
get_plugin_class->>plugin_pool: get_plugin("MyPluginName")
activate plugin_pool
plugin_pool-->>get_plugin_class: PluginClass
deactivate plugin_pool
else entry is already a class type
get_plugin_class-->>setup(): PluginClass (direct return)
end
get_plugin_class-->>setup(): Resolved PluginClass
deactivate get_plugin_class
end
setup()-->>System: Setup complete, allowed_plugin_types initialized
deactivate setup()
Sequence Diagram: Plugin Registration Uniqueness Check in update_plugin_poolsequenceDiagram
participant "update_plugin_pool()" as update_plugin_pool
participant "components_registry" as components_registry
participant "plugin_pool"
activate update_plugin_pool
update_plugin_pool->>components_registry: Iterates items (key, (config, plugin, slot_plugins))
loop for each component plugin
update_plugin_pool->>plugin_pool: Check if plugin.__name__ in plugin_pool.plugins
alt plugin name NOT in plugin_pool
update_plugin_pool->>plugin_pool: register_plugin(plugin)
activate plugin_pool
plugin_pool-->>update_plugin_pool: Registered
deactivate plugin_pool
opt if slot_plugins exist
update_plugin_pool->>plugin_pool: register_plugin(slot_plugin)
activate plugin_pool
plugin_pool-->>update_plugin_pool: Registered
deactivate plugin_pool
end
else plugin name ALREADY in plugin_pool
update_plugin_pool->>update_plugin_pool: Raise ImproperlyConfigured (duplicate plugin name)
end
end
deactivate update_plugin_pool
Class Diagram: Changes to Plugin Configuration and Registration LogicclassDiagram
class `djangocms_frontend.plugin_tag` {
<<Module>>
+get_plugin_class(settings_string: str | type) type
+setup() void
}
note for `djangocms_frontend.plugin_tag.get_plugin_class` "New function. Resolves plugin class:\n- String & contains '.': uses importlib (dotted path)\n- String & no '.': uses plugin_pool.get_plugin() (name)\n- Type: returns directly"
note for `djangocms_frontend.plugin_tag.setup` "Modified: Initializes `allowed_plugin_types` using `get_plugin_class()`."
class `djangocms_frontend.cms_plugins` {
<<Module>>
+update_plugin_pool() void
}
note for `djangocms_frontend.cms_plugins.update_plugin_pool` "Modified: Enforces unique plugin names.\nIterates `components._registry.items()`.
Raises `ImproperlyConfigured` if `plugin.__name__` already in `plugin_pool.plugins`."
class `djangocms_frontend.component_base.FrontendUIItem` {
+plugin_factory(cls) type
}
note for `djangocms_frontend.component_base.FrontendUIItem.plugin_factory` "Modified: Sets `__module__` of the dynamically generated plugin to 'djangocms_frontend.cms_plugins'."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #279 +/- ##
==========================================
- Coverage 88.68% 88.56% -0.12%
==========================================
Files 124 124
Lines 3366 3375 +9
Branches 283 285 +2
==========================================
+ Hits 2985 2989 +4
- Misses 264 267 +3
- Partials 117 119 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fsbraun - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Guard against empty allowed_plugin_types to avoid TypeError (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 5 other issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -65,7 +60,21 @@ def patch_template(template): | |||
| return copied_template if patch else template | |||
|
|
|||
|
|
|||
| def get_plugin_class(settings_string: str | type) -> type: | |||
There was a problem hiding this comment.
nitpick: Union type annotation requires Python 3.10+
If you need to support Python <3.10, replace str | type with Union[str, Type] or enable from __future__ import annotations.
| """Get the plugin class from the settings string or import it if it's a dotted path.""" | ||
| if isinstance(settings_string, str): | ||
| if "." in settings_string: | ||
| # import the class if a dotted oath is given |
There was a problem hiding this comment.
nitpick (typo): Typo in comment: 'oath' should be 'path'
Suggested change
| # import the class if a dotted oath is given | |
| # import the class if a dotted path is given |
| if "." in settings_string: | ||
| # import the class if a dotted oath is given | ||
| module_name, class_name = settings_string.rsplit(".", 1) | ||
| return getattr(importlib.import_module(module_name), class_name, None) |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid returning None on failed import
Omit the default in getattr so it raises an AttributeError, or explicitly raise ImportError, to fail fast instead of returning None.
Suggested change
| return getattr(importlib.import_module(module_name), class_name, None) | |
| return getattr(importlib.import_module(module_name), class_name) |
| def setup(): | ||
| allowed_plugin_types = tuple(get_plugin_class(cls) for cls in getattr(settings, "CMS_COMPONENT_PLUGINS", [])) | ||
|
|
||
| for plugin in plugin_pool.get_all_plugins(): | ||
| if not issubclass(plugin, allowed_plugin_types): |
There was a problem hiding this comment.
issue (bug_risk): Guard against empty allowed_plugin_types to avoid TypeError
When CMS_COMPONENT_PLUGINS is empty, allowed_plugin_types becomes () and issubclass(plugin, ()) raises a TypeError. Either skip the check when it's empty or default to (object,).
Comment on lines
25
to
27
| globals()[slot_plugin.__name__] = slot_plugin | ||
| # Register the slot plugin with the plugin pool | ||
| plugin_pool.register_plugin(slot_plugin) |
There was a problem hiding this comment.
suggestion (bug_risk): Also handle name conflicts for slot_plugins
Currently only main plugin collisions are caught, so slot_plugins with duplicate names are silently re-registered. Add conflict checks and error handling for each slot_plugin.
Suggested change
| globals()[slot_plugin.__name__] = slot_plugin | |
| # Register the slot plugin with the plugin pool | |
| plugin_pool.register_plugin(slot_plugin) | |
| # Register slot plugins, checking for name conflicts | |
| for slot_plugin in slot_plugins: | |
| if slot_plugin.__name__ not in plugin_pool.plugins: | |
| globals()[slot_plugin.__name__] = slot_plugin | |
| plugin_pool.register_plugin(slot_plugin) | |
| else: | |
| raise ImproperlyConfigured( | |
| f"Cannot register slot plugin {slot_plugin.__name__} " | |
| f"since a plugin {slot_plugin.__name__} is already registered " | |
| f"by {plugin_pool.plugins[slot_plugin.__name__].__module__}." | |
| ) |
| includes the necessary plugin classes and their subclasses. This setting | ||
| allows you to specify which plugins can be used directly in templates | ||
| is a list that includes the necessary plugin names or dotted path to | ||
| a plugin parent class . Only plugins named in the listing or their |
There was a problem hiding this comment.
issue (typo): Typo: extra space before period.
Remove the extra space before the period after 'class' so it reads 'class. Only'.
Suggested change
| a plugin parent class . Only plugins named in the listing or their | |
| a plugin parent class. Only plugins named in the listing or their |
| @@ -11,7 +12,7 @@ def update_plugin_pool(): | |||
| from .component_pool import components | |||
|
|
|||
| # Loop through the values in the components' registry | |||
| for _, plugin, slot_plugins in components._registry.values(): | |||
| for key, (_, plugin, slot_plugins) in components._registry.items(): | |||
| if plugin.__name__ not in plugin_pool.plugins: | |||
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Swap if/else branches (
swap-if-else-branches) - Remove unnecessary else after guard condition (
remove-unnecessary-else)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To simplify the configuration used for component plug-ins,
CMS_COMPONENT_PLUGINSnow accepts entries with a plug-in name in addition to a dotted path. It will look up the plug-in on the plug-in pool.Tests are coming up.