feat(plugin): enhance register method to support aliases for plugins #5162
Conversation
📝 WalkthroughWalkthroughAn alias mechanism is introduced to the plugin registration API. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the plugin registration system to support alias keys, allowing plugins to be registered under multiple names for better flexibility and backward compatibility.
Changes:
- Added optional
aliasparameter toPlugin.register()andPR.register()methods - Updated method signatures to accept a list of alternative keys for plugins
- Enhanced documentation with examples showing alias usage
Comments suppressed due to low confidence (1)
deepmd/utils/plugin.py:144
- The docstring still contains outdated references to "descriptor plugin" and "registered descriptor", but this is a generic plugin registry function used for various plugin types (backend, model, fitting, etc.). Update the documentation to be more generic, such as "Register a plugin" and "the registered plugin" to accurately reflect its general-purpose nature.
"""Register a descriptor plugin.
Parameters
----------
key : str
The primary key of the plugin.
alias : list[str], optional
Alternative keys for the plugin.
Returns
-------
callable[[object], object]
the registered descriptor
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.plugins[key] = object | ||
| if alias: | ||
| for alias_key in alias: | ||
| self.plugins[alias_key] = object |
There was a problem hiding this comment.
The code doesn't validate whether the primary key already exists in the plugins dictionary before registration. If a plugin is registered with a key that already exists (or matches an existing alias), it will silently overwrite the previous plugin. Consider adding validation to check for key conflicts before registration.
| self.plugins[key] = object | |
| if alias: | |
| for alias_key in alias: | |
| self.plugins[alias_key] = object | |
| # Collect all keys (primary and aliases) to be registered | |
| keys_to_register: list[str] = [key] | |
| if alias: | |
| keys_to_register.extend(alias) | |
| # Validate that none of the keys are already registered | |
| for k in keys_to_register: | |
| if k in self.plugins: | |
| raise KeyError( | |
| f"Plugin key '{k}' is already registered and cannot be overwritten." | |
| ) | |
| # Register the plugin under all requested keys | |
| for k in keys_to_register: | |
| self.plugins[k] = object |
| for alias_key in alias: | ||
| self.plugins[alias_key] = object |
There was a problem hiding this comment.
The loop variable 'alias_key' creates potential confusion with the parameter 'alias'. Consider using a more descriptive variable name like 'alias_name' or 'key' to improve code clarity and avoid any naming ambiguity.
| for alias_key in alias: | |
| self.plugins[alias_key] = object | |
| for alias_name in alias: | |
| self.plugins[alias_name] = object |
| def decorator(object: object) -> object: | ||
| self.plugins[key] = object | ||
| if alias: | ||
| for alias_key in alias: |
There was a problem hiding this comment.
The alias registration logic doesn't validate whether an alias key already exists in the plugins dictionary. If an alias key is already registered (either as a primary key or another alias), it will be silently overwritten. Consider adding validation to check for key conflicts and either raise an error or log a warning to prevent unintended plugin overwrites.
| for alias_key in alias: | |
| for alias_key in alias: | |
| if alias_key in self.plugins and self.plugins[alias_key] is not object: | |
| raise ValueError( | |
| f"Alias key {alias_key!r} is already registered for a different plugin" | |
| ) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5162 +/- ##
=======================================
Coverage 81.92% 81.92%
=======================================
Files 714 714
Lines 73301 73303 +2
Branches 3616 3617 +1
=======================================
+ Hits 60055 60057 +2
- Misses 12083 12084 +1
+ Partials 1163 1162 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.