fix: various plugin registry improvements - #86
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoves singleton initialization into PluginRegistryBase.new, makes module_prefix/load_plugin/expected_attributes explicit abstract members, changes discovery to derive plugin registration names from module names, tightens attribute validation, and adds example plugin fixtures plus tests for discovery and error paths. Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant Registry as PluginRegistry
participant Discoverer as Module Discoverer
participant Module as Plugin Module
participant Validator as Validator
participant Plugin as Plugin Instance
User->>Registry: instantiate (triggers __new__)
activate Registry
Registry->>Discoverer: collect_plugins() / scan sys.path for module_prefix
activate Discoverer
Discoverer->>Module: import module
Module-->>Discoverer: module
Discoverer-->>Registry: module, derived name
deactivate Discoverer
loop each discovered module
Registry->>Validator: validate_plugin(name, module)
activate Validator
Validator->>Registry: expected_attributes()
Registry-->>Validator: attribute schema
Validator->>Module: read attributes & types
Module-->>Validator: attribute values
alt valid
Validator-->>Registry: valid
Registry->>Registry: register_plugin(name, module)
Registry->>Registry: load_plugin(name, module)
Registry-->>Plugin: plugin instance
else invalid
Validator-->>Registry: raise InvalidPluginException
end
deactivate Validator
end
User->>Registry: get_plugin(plugin_name)
Registry-->>User: TPlugin or InvalidPluginException
deactivate Registry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/example_plugin.py (1)
49-58: Consider defensive handling ofmodule.__file__.Line 56 assumes
module.__file__is not None, which could raise a TypeError if the module is a built-in or namespace package. While test fixtures are file-based, adding a defensive check or assertion would improve robustness.Consider adding a guard:
def load_plugin(self, name: str, module: ModuleType) -> ExamplePlugin: settings_cls = getattr(module, "ExampleSettings", None) string_attr = module.example_string + if module.__file__ is None: + raise ValueError(f"Plugin module {name} has no __file__ attribute") + return ExamplePlugin( _name=name, _settings_cls=settings_cls, file=Path(module.__file__), string_attr=string_attr, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (9)
src/snakemake_interface_common/plugin_registry/__init__.py(5 hunks)tests/__init__.py(1 hunks)tests/example_plugin.py(1 hunks)tests/plugins/invalid-class/snakemake_example_plugin_invalid_class/__init__.py(1 hunks)tests/plugins/invalid-object/snakemake_example_plugin_invalid_object/__init__.py(1 hunks)tests/plugins/missing-attr/snakemake_example_plugin_missing_attr/__init__.py(1 hunks)tests/plugins/valid/snakemake_example_plugin_valid_1/__init__.py(1 hunks)tests/plugins/valid/snakemake_example_plugin_valid_2/__init__.py(1 hunks)tests/test_registry.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
tests/plugins/missing-attr/snakemake_example_plugin_missing_attr/__init__.pytests/plugins/invalid-object/snakemake_example_plugin_invalid_object/__init__.pytests/plugins/valid/snakemake_example_plugin_valid_2/__init__.pytests/plugins/valid/snakemake_example_plugin_valid_1/__init__.pytests/__init__.pytests/example_plugin.pysrc/snakemake_interface_common/plugin_registry/__init__.pytests/plugins/invalid-class/snakemake_example_plugin_invalid_class/__init__.pytests/test_registry.py
🧬 Code graph analysis (5)
tests/plugins/valid/snakemake_example_plugin_valid_1/__init__.py (2)
src/snakemake_interface_common/plugin_registry/plugin.py (1)
SettingsBase(41-50)tests/plugins/invalid-class/snakemake_example_plugin_invalid_class/__init__.py (1)
ExampleSettings(6-7)
tests/example_plugin.py (3)
src/snakemake_interface_common/plugin_registry/__init__.py (1)
PluginRegistryBase(23-206)src/snakemake_interface_common/plugin_registry/plugin.py (2)
PluginBase(82-360)SettingsBase(41-50)src/snakemake_interface_common/plugin_registry/attribute_types.py (3)
AttributeType(17-31)AttributeMode(6-8)AttributeKind(11-13)
src/snakemake_interface_common/plugin_registry/__init__.py (3)
src/snakemake_interface_common/plugin_registry/plugin.py (2)
name(85-85)register_cli_args(134-198)src/snakemake_interface_common/plugin_registry/attribute_types.py (1)
AttributeType(17-31)src/snakemake_interface_common/exceptions.py (1)
InvalidPluginException(77-79)
tests/plugins/invalid-class/snakemake_example_plugin_invalid_class/__init__.py (1)
tests/plugins/valid/snakemake_example_plugin_valid_1/__init__.py (1)
ExampleSettings(9-10)
tests/test_registry.py (4)
src/snakemake_interface_common/plugin_registry/plugin.py (1)
SettingsBase(41-50)src/snakemake_interface_common/exceptions.py (1)
InvalidPluginException(77-79)tests/example_plugin.py (5)
ExamplePlugin(18-34)ExamplePluginRegistry(37-72)new(39-43)name(25-26)settings_cls(33-34)src/snakemake_interface_common/plugin_registry/__init__.py (4)
get_plugin_type(113-125)is_installed(79-81)get_plugin(83-98)get_registered_plugins(75-77)
🔇 Additional comments (23)
tests/__init__.py (1)
1-1: LGTM!Clear comment explaining the purpose of this
__init__.pyfile.src/snakemake_interface_common/plugin_registry/__init__.py (7)
23-42: Excellent documentation.The docstring clearly explains the singleton pattern, naming conventions, discovery mechanism, and includes a concrete example. The note about editable installs is a helpful caveat.
44-56: LGTM!The singleton pattern using
__new__with an initialization guard in__init__is well-implemented. Each subclass will maintain its own_instanceclass attribute due to Python's attribute assignment semantics.
58-72: LGTM!Good organization moving abstract methods to the top. The contract for subclasses is clear and well-documented.
83-98: LGTM!Good improvement to the error message using the pip-installable package name, making it actionable for users.
127-139: LGTM!The plugin discovery logic correctly filters by prefix, transforms underscores to dashes in plugin names, and delegates to
register_pluginfor validation and registration.
141-206: LGTM!The
register_pluginandvalidate_pluginmethods provide thorough validation with clear error messages. The handling of optional vs required attributes and class vs object types is correct.
11-11: Minimum Python version is already properly set to 3.11+.Verification confirms that
snakemake-interface-commonrequires Python >=3.11 in its package metadata (both PyPI and Bioconda). TheSelfimport fromtypingon line 11 is therefore appropriately supported and no additional configuration changes are needed.tests/plugins/valid/snakemake_example_plugin_valid_2/__init__.py (1)
1-3: LGTM!Good test fixture for validating plugin discovery with only required attributes (no optional settings class).
tests/plugins/missing-attr/snakemake_example_plugin_missing_attr/__init__.py (1)
1-1: LGTM!Appropriate negative test fixture for validating error handling when required plugin attributes are missing.
tests/plugins/invalid-class/snakemake_example_plugin_invalid_class/__init__.py (1)
1-7: LGTM!Appropriate negative test fixture for validating error handling when a plugin's class attribute doesn't inherit from the expected base class.
tests/plugins/invalid-object/snakemake_example_plugin_invalid_object/__init__.py (1)
1-3: LGTM! Test fixture correctly implements invalid scenario.This test fixture appropriately provides an integer value for
example_stringto enable validation of type-checking error handling in the registry.tests/plugins/valid/snakemake_example_plugin_valid_1/__init__.py (2)
1-6: LGTM! Valid test fixture structure.The module correctly defines the required
example_stringattribute with the expected string type.
9-10: LGTM! Minimal settings class is appropriate for testing.The empty
ExampleSettingsclass correctly inherits fromSettingsBaseand is sufficient to test the optional settings class attribute validation. The TODO can remain as this fixture validates that minimal settings classes are properly recognized.tests/test_registry.py (5)
1-14: LGTM! Clean test setup.Imports and test directory setup are correct.
16-28: LGTM! Comprehensive basic functionality tests.The test correctly validates registry type inference, singleton behavior, and plugin-not-found error handling.
30-59: LGTM! Thorough plugin discovery and validation tests.The test comprehensively validates plugin discovery, instantiation, and attribute checking for both plugins with and without optional settings classes.
61-72: LGTM! Proper validation of missing attribute handling.The test correctly verifies that the registry raises
InvalidPluginExceptionwith an appropriate error message when a required attribute is missing.
74-98: LGTM! Comprehensive type validation tests.Both
test_invalid_objectandtest_invalid_classproperly validate that the registry rejects plugins with incorrect attribute types and generates informative error messages.tests/example_plugin.py (4)
1-16: LGTM! Clean import structure.All necessary imports are present and correctly organized.
17-35: LGTM! ExamplePlugin correctly implements PluginBase.The dataclass structure and property implementations appropriately fulfill the
PluginBasecontract for testing purposes.
38-48: LGTM! Test registry setup is correct.The
new()classmethod appropriately bypasses singleton behavior for isolated testing, andmodule_prefixcorrectly identifies test plugins.
60-72: LGTM! Attribute specifications are correct.The
expected_attributes()implementation properly defines the validation rules for test plugins, specifyingExampleSettingsas an optional class attribute andexample_stringas a required object attribute.
c205193 to
1ec5805
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/snakemake_interface_common/plugin_registry/__init__.py (1)
135-137: Preserve the package-name filter before importing.
is_valid_plugin_package_name()is never consulted anywhere in this class, so any subclass override is currently dead and rejected packages still get imported. If that hook is meant to remain part of the extension surface, gate on it here beforeimport_module().Possible fix
name = moduleinfo.name.removeprefix(self.module_prefix).replace("_", "-") + if not self.is_valid_plugin_package_name(name): + continue module = importlib.import_module(moduleinfo.name) self.register_plugin(name, module)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/snakemake_interface_common/plugin_registry/__init__.py` around lines 135 - 137, The code currently imports every discovered module before checking the package-name filter, so override hooks like is_valid_plugin_package_name are never used; modify the loop that handles moduleinfo (using moduleinfo.name and self.module_prefix) to compute the plugin name (using moduleinfo.name.removeprefix(self.module_prefix).replace("_", "-")) and call self.is_valid_plugin_package_name(name) before calling importlib.import_module(moduleinfo.name); only call importlib.import_module and then self.register_plugin(name, module) if the name passes the filter, ensuring subclasses can override is_valid_plugin_package_name to reject packages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/snakemake_interface_common/plugin_registry/__init__.py`:
- Around line 47-54: The singleton currently calls collect_plugins() from
__new__, which triggers plugin discovery before subclass __init__ runs and
causes __init__ to be re-entered on cached singletons; move the one-time
collection out of __new__ and into a guarded post-init path instead: remove
collect_plugins() from __new__, add a boolean flag like _plugins_collected on
the class/instance, and call collect_plugins() from __init__ only when not
already collected (or expose an explicit initialize_plugins() method and call it
once); ensure you keep the singleton behavior using cls._instance but prevent
re-running collect_plugins and avoid changing subclass init semantics (refer to
methods __new__, __init__, collect_plugins, and attribute
_instance/_plugins_collected when applying the change).
---
Nitpick comments:
In `@src/snakemake_interface_common/plugin_registry/__init__.py`:
- Around line 135-137: The code currently imports every discovered module before
checking the package-name filter, so override hooks like
is_valid_plugin_package_name are never used; modify the loop that handles
moduleinfo (using moduleinfo.name and self.module_prefix) to compute the plugin
name (using moduleinfo.name.removeprefix(self.module_prefix).replace("_", "-"))
and call self.is_valid_plugin_package_name(name) before calling
importlib.import_module(moduleinfo.name); only call importlib.import_module and
then self.register_plugin(name, module) if the name passes the filter, ensuring
subclasses can override is_valid_plugin_package_name to reject packages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bc90299-57ba-40a7-a90d-758d8ea1c93d
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (10)
src/snakemake_interface_common/plugin_registry/__init__.pytests/__init__.pytests/example_plugin.pytests/plugins/invalid-class/snakemake_example_plugin_invalid_class/__init__.pytests/plugins/invalid-object/snakemake_example_plugin_invalid_object/__init__.pytests/plugins/missing-attr/snakemake_example_plugin_missing_attr/__init__.pytests/plugins/valid/snakemake_example_plugin_valid_1/__init__.pytests/plugins/valid/snakemake_example_plugin_valid_2/__init__.pytests/test_registry.pytests/tests.py
✅ Files skipped from review due to trivial changes (6)
- tests/plugins/missing-attr/snakemake_example_plugin_missing_attr/init.py
- tests/init.py
- tests/plugins/valid/snakemake_example_plugin_valid_2/init.py
- tests/plugins/invalid-object/snakemake_example_plugin_invalid_object/init.py
- tests/plugins/invalid-class/snakemake_example_plugin_invalid_class/init.py
- tests/plugins/valid/snakemake_example_plugin_valid_1/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_registry.py
f5ee7ca to
9a6339f
Compare
9a6339f to
29baf79
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_registry.py (2)
26-27: Assert the error message content to cover theget_plugin()message fix.This currently validates only exception type. Add a message assertion so the PR’s package-name error-message correction is explicitly tested.
Proposed assertion update
- with pytest.raises(InvalidPluginException): + with pytest.raises(InvalidPluginException) as exc_info: registry.get_plugin("foo") + assert ExamplePluginRegistry.get_plugin_package_name() in str(exc_info.value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_registry.py` around lines 26 - 27, The test currently only checks that registry.get_plugin("foo") raises InvalidPluginException; update the pytest.raises context to also assert the exception message contains the corrected package-name text by capturing the exception (e.g., with "with pytest.raises(InvalidPluginException) as excinfo: registry.get_plugin('foo')") and then assert the message (e.g., assert "expected package-name error text" in str(excinfo.value)). Reference the existing test call to registry.get_plugin and the InvalidPluginException class when adding the message assertion.
16-22: Isolate singleton state between tests to prevent order-dependent behavior.
Line 18initializesExamplePluginRegistry’s singleton cache, which can leak across tests/modules. Consider resetting_instancein anautousefixture to keep tests hermetic.Proposed test-isolation patch
+@pytest.fixture(autouse=True) +def _reset_example_registry_singleton(): + ExamplePluginRegistry._instance = None + yield + ExamplePluginRegistry._instance = None + def test_basic():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_registry.py` around lines 16 - 22, The test leaks the ExamplePluginRegistry singleton between tests; modify the test suite to reset ExamplePluginRegistry._instance before each test using an autouse fixture so tests are hermetic: add an autouse pytest fixture (e.g., in tests/test_registry.py or conftest.py) that saves any existing ExamplePluginRegistry._instance, sets ExamplePluginRegistry._instance = None (or the desired fresh state) before each test run and restores it after, ensuring test_basic and other tests always get a fresh ExamplePluginRegistry instance and do not depend on test order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_registry.py`:
- Around line 26-27: The test currently only checks that
registry.get_plugin("foo") raises InvalidPluginException; update the
pytest.raises context to also assert the exception message contains the
corrected package-name text by capturing the exception (e.g., with "with
pytest.raises(InvalidPluginException) as excinfo: registry.get_plugin('foo')")
and then assert the message (e.g., assert "expected package-name error text" in
str(excinfo.value)). Reference the existing test call to registry.get_plugin and
the InvalidPluginException class when adding the message assertion.
- Around line 16-22: The test leaks the ExamplePluginRegistry singleton between
tests; modify the test suite to reset ExamplePluginRegistry._instance before
each test using an autouse fixture so tests are hermetic: add an autouse pytest
fixture (e.g., in tests/test_registry.py or conftest.py) that saves any existing
ExamplePluginRegistry._instance, sets ExamplePluginRegistry._instance = None (or
the desired fresh state) before each test run and restores it after, ensuring
test_basic and other tests always get a fresh ExamplePluginRegistry instance and
do not depend on test order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04de27b4-55be-4d1f-ba65-3805c7b4a5c2
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (10)
src/snakemake_interface_common/plugin_registry/__init__.pytests/__init__.pytests/example_plugin.pytests/plugins/invalid-class/snakemake_example_plugin_invalid_class/__init__.pytests/plugins/invalid-object/snakemake_example_plugin_invalid_object/__init__.pytests/plugins/missing-attr/snakemake_example_plugin_missing_attr/__init__.pytests/plugins/valid/snakemake_example_plugin_valid_1/__init__.pytests/plugins/valid/snakemake_example_plugin_valid_2/__init__.pytests/test_registry.pytests/tests.py
✅ Files skipped from review due to trivial changes (7)
- tests/plugins/missing-attr/snakemake_example_plugin_missing_attr/init.py
- tests/init.py
- tests/plugins/valid/snakemake_example_plugin_valid_2/init.py
- tests/tests.py
- tests/plugins/valid/snakemake_example_plugin_valid_1/init.py
- tests/plugins/invalid-class/snakemake_example_plugin_invalid_class/init.py
- tests/plugins/invalid-object/snakemake_example_plugin_invalid_object/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/example_plugin.py
- src/snakemake_interface_common/plugin_registry/init.py
90c5aad to
82c33a8
Compare
|
Followed CodeRabbit's suggestion about properly resetting the singleton of |
cademirch
changed the title
Co-authored-by: Cade Mirchandani <cmirchan@ucsc.edu>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/snakemake_interface_common/plugin_registry/__init__.py`:
- Around line 157-158: The line containing "name =
name.removeprefix(self.module_prefix).replace("_", "-")" is under-indented and
causes an IndentationError on import; adjust its indentation to match the
surrounding method body (same level as the following "if name in self.plugins:"
line) so it is inside the method scope, ensuring the assignment and the
subsequent check against self.plugins are at the same indentation level.
In `@tests/test_registry.py`:
- Line 48: The test declares plugins using the new-style annotation dict[str,
ExamplePlugin], which is incompatible with Python 3.8; change the type
annotation to use typing.Dict (e.g., Dict[str, ExamplePlugin]) or add from
__future__ import annotations at the top of the file to defer evaluation—update
the declaration of plugins in tests/test_registry.py (the plugins variable and
ExamplePlugin type reference) accordingly so it matches the project's Python 3.8
compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a52621e7-efbb-4197-b194-d6feb1eeea8c
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (10)
src/snakemake_interface_common/plugin_registry/__init__.pytests/__init__.pytests/example_plugin.pytests/plugins/invalid-class/snakemake_example_plugin_invalid_class/__init__.pytests/plugins/invalid-object/snakemake_example_plugin_invalid_object/__init__.pytests/plugins/missing-attr/snakemake_example_plugin_missing_attr/__init__.pytests/plugins/valid/snakemake_example_plugin_valid_1/__init__.pytests/plugins/valid/snakemake_example_plugin_valid_2/__init__.pytests/test_registry.pytests/tests.py
✅ Files skipped from review due to trivial changes (7)
- tests/plugins/missing-attr/snakemake_example_plugin_missing_attr/init.py
- tests/init.py
- tests/plugins/invalid-object/snakemake_example_plugin_invalid_object/init.py
- tests/plugins/invalid-class/snakemake_example_plugin_invalid_class/init.py
- tests/tests.py
- tests/plugins/valid/snakemake_example_plugin_valid_1/init.py
- tests/plugins/valid/snakemake_example_plugin_valid_2/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/example_plugin.py
| name = name.removeprefix(self.module_prefix).replace("_", "-") | ||
| if name in self.plugins: |
There was a problem hiding this comment.
Critical indentation error will cause IndentationError on import.
Line 157 has only a single space of indentation instead of matching the method body indentation (8 spaces). This is a syntax error that will prevent the module from loading.
🐛 Proposed fix for indentation
- """
- name = name.removeprefix(self.module_prefix).replace("_", "-")
+ """
+ name = name.removeprefix(self.module_prefix).replace("_", "-")
if name in self.plugins:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/snakemake_interface_common/plugin_registry/__init__.py` around lines 157
- 158, The line containing "name =
name.removeprefix(self.module_prefix).replace("_", "-")" is under-indented and
causes an IndentationError on import; adjust its indentation to match the
surrounding method body (same level as the following "if name in self.plugins:"
line) so it is inside the method scope, ensuring the assignment and the
subsequent check against self.plugins are at the same indentation level.
| registry = ExamplePluginRegistry() | ||
|
|
||
| expected_plugins = {"valid-1", "valid-2"} | ||
| plugins: dict[str, ExamplePlugin] = {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check minimum Python version requirement in project configuration
echo "=== Checking pyproject.toml for Python version ==="
cat pyproject.toml 2>/dev/null | grep -A5 -i "python" | head -20
echo ""
echo "=== Checking setup.py/setup.cfg for Python version ==="
cat setup.py 2>/dev/null | grep -i "python"
cat setup.cfg 2>/dev/null | grep -i "python"
echo ""
echo "=== Checking for other uses of lowercase generics in tests ==="
rg -n ":\s*(list|dict|set|tuple)\[" tests/ --type pyRepository: snakemake/snakemake-interface-common
Length of output: 895
🏁 Script executed:
#!/bin/bash
echo "=== Checking main module for Dict import from typing ==="
find snakemake_interface_common -name "*.py" -type f | head -10 | xargs grep -l "from typing import" | head -5 | xargs grep -n "Dict" | head -20
echo ""
echo "=== Checking test_registry.py imports ==="
head -30 tests/test_registry.py
echo ""
echo "=== Checking for Dict usage in main module ==="
rg -n "Dict\[" snakemake_interface_common/ --type py | head -20
echo ""
echo "=== Checking lowercase generic usage across main module ==="
rg -n ":\s*(list|dict|set|tuple)\[" snakemake_interface_common/ --type py | head -20Repository: snakemake/snakemake-interface-common
Length of output: 1331
🏁 Script executed:
#!/bin/bash
echo "=== Repository structure ==="
ls -la
echo ""
echo "=== Find all Python source directories ==="
find . -maxdepth 2 -type d -name "*snakemake*" | head -10
echo ""
echo "=== Check for main source files ==="
find . -maxdepth 2 -name "*.py" -type f | head -20
echo ""
echo "=== Check for Dict usage anywhere in project ==="
rg -n "Dict\[" --type py | head -30
echo ""
echo "=== Check lowercase generic usage in entire project ==="
rg -n ":\s*(list|dict|set|tuple)\[" --type py | head -30Repository: snakemake/snakemake-interface-common
Length of output: 2176
🏁 Script executed:
#!/bin/bash
echo "=== Full imports section of test_registry.py ==="
head -20 tests/test_registry.py
echo ""
echo "=== Check for __future__ import ==="
rg -n "from __future__" tests/test_registry.py
echo ""
echo "=== Check typing imports in test_registry.py ==="
rg -n "from typing import\|import typing" tests/test_registry.py
echo ""
echo "=== Verify Dict usage in main module ==="
rg -n "Dict\[" src/snakemake_interface_common/ --type pyRepository: snakemake/snakemake-interface-common
Length of output: 1783
Use Dict[str, ExamplePlugin] for Python 3.8 compatibility.
dict[str, ExamplePlugin] requires Python 3.9+, but the project declares requires-python = ">=3.8" in pyproject.toml and the main module consistently uses Dict from typing. Either use Dict[str, ExamplePlugin] or add from __future__ import annotations at the top of the file to maintain compatibility with Python 3.8.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_registry.py` at line 48, The test declares plugins using the
new-style annotation dict[str, ExamplePlugin], which is incompatible with Python
3.8; change the type annotation to use typing.Dict (e.g., Dict[str,
ExamplePlugin]) or add from __future__ import annotations at the top of the file
to defer evaluation—update the declaration of plugins in tests/test_registry.py
(the plugins variable and ExamplePlugin type reference) accordingly so it
matches the project's Python 3.8 compatibility.
|
@cademirch applied your suggestion. One last potential change - I'm wondering if it might be better to use a metaclass to implement the singleton pattern rather than using I have a PR for it here. |
johanneskoester
changed the title
3 participants
PluginRegsitryBase:__new__()- previous annotation allowed for returning an instance of a different registry class as long as the type parameter was consistent. This may have been added in the first place due to thereturn cls._instancestatement, which type checkers might not be able to resolve correctly. Fixed instead by adding# type: ignoreto return statement, which should cause type checkers to infer standard behavior of returning an instance ofcls.collect_plugins()in__new__()rather than__init__(): previously_instancewas assigned prior tocollect_plugins()being called, so if there were any errors the singleton instance would be left in a broken or partially initialized state. This does not catch errors in__init__()if any subclasses define it, from a quick survey of the actual plugin packages I don't think any do.register_plugin()method: Fixed issue where name conflicts were checked using the full module name with prefix instead of the registry name. Changed argument name to be a bit more clear.get_plugin()method: Fix error message to properly display expected package name usingget_plugin_package_name(). Also corrected return type annotation.validate_plugin()method: fixTypeErrorwhen checking a type attribute where the value is not actually a type.tests/to PixiformattaskTestPluginandTestSettingsclasses.Summary by CodeRabbit
Bug Fixes
New Features
Tests