Skip to content

Add lazy-loaded Patch/Spool method namespace plugins - #617

Merged
d-chambers merged 8 commits into
masterfrom
registerable_namespace
Mar 12, 2026
Merged

Add lazy-loaded Patch/Spool method namespace plugins#617
d-chambers merged 8 commits into
masterfrom
registerable_namespace

Conversation

@d-chambers

@d-chambers d-chambers commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds a plugin system for MethodNameSpace on Patch and BaseSpool, with lazy loading through entry points.

Third-party packages can now register patch/spool namespaces in pyproject.toml, and DASCore will only import the plugin when that namespace attribute is first accessed. Entry points are cached, but plugin code is not
eagerly loaded, so startup time is not affected.

Summary by CodeRabbit

  • New Features

    • Dynamic, plugin-backed namespaces for patches and spools enabling lazy-loading extensions
    • Utilities to discover, load, and cache plugin entry points for extension loading
  • Refactor

    • Unified namespace framework replacing legacy namespace machinery
    • Visualization and I/O namespace wiring simplified to use the new system
  • Documentation

    • Added guide on extending the project with namespaces and plugins
  • Tests

    • Added comprehensive tests for namespace behavior and plugin loading/caching

@coderabbitai

coderabbitai Bot commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Patch and BaseSpool now inherit NamespaceOwner to enable lazy, entry-point-backed namespaces. Legacy MethodNameSpace machinery was removed and replaced by a new namespace module and plugin loader utilities. IO and viz namespace classes were retargeted to the new namespace types; tests, docs, and pyproject entry-points were added/updated.

Changes

Cohort / File(s) Summary
Core objects
dascore/core/patch.py, dascore/core/spool.py
Patch and BaseSpool now subclass NamespaceOwner and set _namespace_entry_point_group; removed Patch.viz/Patch.io properties and added spool _namespace_attr_errors guidance.
Namespace implementation
dascore/utils/namespace.py
New namespace system: _NameSpaceMeta, _MethodNameSpace, PatchNameSpace, SpoolNameSpace, NamespaceOwner, host-method rebinding, registry, and lazy resolution.
Plugin loaders
dascore/utils/plugins.py, dascore/io/core.py
Added cached get_entry_point_loaders() and maybe_load_entry_point(); IO code now uses these helpers instead of direct importlib.entry_points calls.
IO & viz wiring
dascore/io/__init__.py, dascore/viz/__init__.py
PatchIO/VizPatchNameSpace now derive from PatchNameSpace; added name = "io" and name = "viz" and bound viz helpers into the namespace.
Legacy cleanup
dascore/utils/misc.py, tests/test_utils/test_misc.py
Removed old MethodNameSpace/metaclass and associated test scaffolding.
Tests, docs & packaging
tests/test_utils/test_namespace.py, tests/test_utils/test_plugins.py, docs/contributing/extending_dascore.qmd, pyproject.toml, scripts/_templates/_quarto.yml
Added comprehensive namespace/plugin tests, documentation for extending via namespaces, added dascore.patch_namespace entry-point mappings (io, viz) in pyproject, and updated docs navigation.

Possibly related PRs

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 78.95% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description is concise but missing details required by the template. It lacks: issue reference, documentation confirmation, testing confirmation, and ready_for_review tag checklist items. Add a checklist section confirming which documentation, testing, and contribution guidelines have been followed, and reference any related issues.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main feature: adding lazy-loaded namespace plugins for Patch and Spool classes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch registerable_namespace

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added proc Related to processing module patch related to Patch class labels Mar 10, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
dascore/core/spool.py (1)

117-132: Consider removing redundant viz handling in __getattr__.

The viz property at lines 360-369 raises the same AttributeError with guidance. Since properties are resolved before __getattr__ is called, the viz-specific handling here (lines 123-130) will never execute for BaseSpool instances.

However, this could serve as a fallback for subclasses that might shadow the property differently, so keeping it is acceptable if that's the intent.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dascore/core/spool.py` around lines 117 - 132, Remove the redundant
special-case handling for "viz" inside the __getattr__ method: delete the if
item == "viz": ... block (the custom AttributeError message) so __getattr__ only
attempts to load the namespace via
self.__class__._namespace_manager.load_plugin(item) and otherwise raises the
generic AttributeError; this leaves property-based viz resolution (defined
elsewhere on BaseSpool) intact and avoids unreachable code while preserving the
descriptor lookup and fallback behavior in __getattr__.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@dascore/utils/namespace.py`:
- Around line 90-92: The return line in get_registered_method_namespaces is over
the 88-char limit; fix it by splitting the expression into smaller parts—e.g.,
first assign getattr(owner_cls, "_method_namespace_registry", {}) to a short
variable name like registry and then return dict(registry); reference the
function get_registered_method_namespaces and the attribute
_method_namespace_registry when locating where to change.

In `@tests/test_utils/test_namespace.py`:
- Around line 212-222: The failing line exceeds the 88-char limit; break the
long assertion into multiple shorter lines so it fits the linter. For example,
assign the plugin name to a local variable or wrap the method call in
parentheses and split across lines when invoking
Patch._namespace_manager.load_plugin("lazy_patch_loaded") (references: Patch,
_namespace_manager, load_plugin, _method_namespace_registry).

---

Nitpick comments:
In `@dascore/core/spool.py`:
- Around line 117-132: Remove the redundant special-case handling for "viz"
inside the __getattr__ method: delete the if item == "viz": ... block (the
custom AttributeError message) so __getattr__ only attempts to load the
namespace via self.__class__._namespace_manager.load_plugin(item) and otherwise
raises the generic AttributeError; this leaves property-based viz resolution
(defined elsewhere on BaseSpool) intact and avoids unreachable code while
preserving the descriptor lookup and fallback behavior in __getattr__.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8826a422-2a8c-47d7-b8fe-5bb3f55c27a7

📥 Commits

Reviewing files that changed from the base of the PR and between 5de00c6 and 6790b24.

📒 Files selected for processing (8)
  • dascore/core/patch.py
  • dascore/core/spool.py
  • dascore/io/__init__.py
  • dascore/utils/misc.py
  • dascore/utils/namespace.py
  • dascore/viz/__init__.py
  • tests/test_utils/test_misc.py
  • tests/test_utils/test_namespace.py
💤 Files with no reviewable changes (2)
  • tests/test_utils/test_misc.py
  • dascore/utils/misc.py

Comment thread dascore/utils/namespace.py Outdated
Comment thread tests/test_utils/test_namespace.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6790b24e3c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread dascore/utils/misc.py
super().__setattr__(key, value)


@contextlib.contextmanager

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve MethodNameSpace export in utils.misc

This change removes MethodNameSpace from dascore.utils.misc without a compatibility alias, so any existing extension code that does from dascore.utils.misc import MethodNameSpace will now fail at import time before it can register namespaces. Because this class was previously used directly (including in this repo’s tests before this commit), keeping a re-export/deprecation shim in misc.py would avoid breaking downstream plugins on upgrade.

Useful? React with 👍 / 👎.

Comment thread dascore/core/spool.py Outdated
def __getattr__(self, item):
"""Try loading a lazily registered namespace before failing."""
manager = self.__class__._namespace_manager
if manager.load_plugin(item):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle reserved viz name before lazy plugin loading

BaseSpool.__getattr__ attempts manager.load_plugin(item) before the special-case viz error path, so an installed entry point named viz will trigger register_method_namespace against an already-existing viz attribute and raise ParameterError instead of the intended AttributeError guidance. This makes plain spool.viz access fail with an unexpected exception when a plugin uses that name.

Useful? React with 👍 / 👎.

@codecov

codecov Bot commented Mar 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.48%. Comparing base (1cc21db) to head (fdcae7c).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #617   +/-   ##
=======================================
  Coverage   99.48%   99.48%           
=======================================
  Files         133      135    +2     
  Lines       11494    11558   +64     
=======================================
+ Hits        11435    11499   +64     
  Misses         59       59           
Flag Coverage Δ
unittests 99.48% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot added the documentation Improvements or additions to documentation label Mar 11, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
dascore/utils/plugins.py (1)

16-22: Consider more specific return type hint.

The function returns the result of invoking the loader, which could be any class. The Any return type is acceptable, but you might consider documenting in the docstring that it returns the loaded entry-point target (typically a class).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dascore/utils/plugins.py` around lines 16 - 22, Update the
maybe_load_entry_point signature to use a more specific return type (e.g.,
Optional[Type[Any]] or Optional[Any] if you want to allow non-class callables)
instead of plain Any, and update the docstring to state that it returns the
loaded entry-point target (typically a class) or None; also add the necessary
typing imports (Type, Optional) and keep the function body unchanged (refer to
maybe_load_entry_point and get_entry_point_loaders to locate the code).
dascore/core/spool.py (1)

61-68: Redundant error handling for viz attribute.

The _namespace_attr_errors["viz"] dictionary entry (lines 61-68) and the explicit viz property (lines 338-346) contain identical error messages. Since properties take precedence over __getattr__ in Python's attribute lookup, the dictionary entry will never be used for the viz attribute.

Consider removing one of these to reduce redundancy:

♻️ Option 1: Remove the explicit property (rely on __getattr__)
-    `@property`
-    def viz(self):
-        """Raise AttributeError when Spool.viz is accessed."""
-        msg = (
-            "'Spool' has no 'viz' namespace. "
-            "Apply 'viz' on a Patch object. "
-            "(you can merge a subset of the spool into a single patch using "
-            "the Chunk function. i.e., spool.chunk(time=None)[0].viz.waterfall())"
-        )
-        raise AttributeError(msg)
♻️ Option 2: Remove the dict entry (keep explicit property)
     _namespace_entry_point_group = "dascore.spool_namespace"
-    _namespace_attr_errors: ClassVar[dict[str, str]] = {
-        "viz": (
-            "'Spool' has no 'viz' namespace. "
-            "Apply 'viz' on a Patch object. "
-            "(you can merge a subset of the spool into a single patch using "
-            "the Chunk function. i.e., spool.chunk(time=None)[0].viz.waterfall())"
-        )
-    }
+    _namespace_attr_errors: ClassVar[dict[str, str]] = {}

Also applies to: 338-346

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dascore/core/spool.py` around lines 61 - 68, The error message for the 'viz'
attribute is duplicated: remove one of the two definitions to eliminate
redundancy—either delete the "viz" key from the _namespace_attr_errors dict or
remove the explicit viz property (the property defined as viz on the Spool
class), keeping the other so attribute lookup via __getattr__ still returns the
intended message; ensure whichever you keep produces the identical error text
and update any tests or references to rely on the retained implementation.
tests/test_utils/test_namespace.py (1)

16-48: Avoid mutating the global namespace registry at import time.

These helper classes register themselves in _MethodNameSpace._registry as soon as the test module is imported, so this file leaves shared state behind for the rest of the test session. That makes namespace tests more order-dependent than they need to be. Consider creating these classes inside the tests that need them, or snapshot/restoring _MethodNameSpace._registry with a fixture.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_utils/test_namespace.py` around lines 16 - 48, The helper classes
ParentClass, ParentClassNamespace, Namespace1, and Namespace2 are registering
themselves into _MethodNameSpace._registry at import time; move their
definitions out of module scope and into the individual tests that require them
(or add a fixture that snapshots _MethodNameSpace._registry before test, yields,
and restores it after) so the global _MethodNameSpace._registry is not mutated
on import; ensure any test that needs these classes either defines them locally
or uses the fixture to prevent shared-state leakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@dascore/utils/namespace.py`:
- Around line 95-98: get_registered_namespaces currently returns the live
mutable bucket from _MethodNameSpace._registry keyed by
cls._namespace_entry_point_group, which allows callers to mutate global state
and can create a new bucket due to defaultdict; change get_registered_namespaces
to fetch the group with
_MethodNameSpace._registry.get(cls._namespace_entry_point_group, {}) to avoid
creating a new default entry and return a shallow copy (e.g., dict(...)) so
callers get a snapshot rather than the live dict.
- Around line 55-73: The __init_subclass__ implementation should invoke the
parent cooperative initializer to preserve multiple-inheritance behavior; add a
call to super().__init_subclass__(**kwargs) at the start of the
__init_subclass__ method so any parent classes' __init_subclass__ logic runs,
then proceed with the existing wrapping and registration logic that touches
cls.name, cls.entry_point_group, and cls._registry to avoid breaking cooperative
inheritance.

---

Nitpick comments:
In `@dascore/core/spool.py`:
- Around line 61-68: The error message for the 'viz' attribute is duplicated:
remove one of the two definitions to eliminate redundancy—either delete the
"viz" key from the _namespace_attr_errors dict or remove the explicit viz
property (the property defined as viz on the Spool class), keeping the other so
attribute lookup via __getattr__ still returns the intended message; ensure
whichever you keep produces the identical error text and update any tests or
references to rely on the retained implementation.

In `@dascore/utils/plugins.py`:
- Around line 16-22: Update the maybe_load_entry_point signature to use a more
specific return type (e.g., Optional[Type[Any]] or Optional[Any] if you want to
allow non-class callables) instead of plain Any, and update the docstring to
state that it returns the loaded entry-point target (typically a class) or None;
also add the necessary typing imports (Type, Optional) and keep the function
body unchanged (refer to maybe_load_entry_point and get_entry_point_loaders to
locate the code).

In `@tests/test_utils/test_namespace.py`:
- Around line 16-48: The helper classes ParentClass, ParentClassNamespace,
Namespace1, and Namespace2 are registering themselves into
_MethodNameSpace._registry at import time; move their definitions out of module
scope and into the individual tests that require them (or add a fixture that
snapshots _MethodNameSpace._registry before test, yields, and restores it after)
so the global _MethodNameSpace._registry is not mutated on import; ensure any
test that needs these classes either defines them locally or uses the fixture to
prevent shared-state leakage.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 989181f4-cb15-4221-ae3d-a01e81651c20

📥 Commits

Reviewing files that changed from the base of the PR and between 6790b24 and cece2ba.

📒 Files selected for processing (12)
  • dascore/core/patch.py
  • dascore/core/spool.py
  • dascore/io/__init__.py
  • dascore/io/core.py
  • dascore/utils/namespace.py
  • dascore/utils/plugins.py
  • dascore/viz/__init__.py
  • docs/contributing/extending_dascore.qmd
  • pyproject.toml
  • scripts/_templates/_quarto.yml
  • tests/test_utils/test_namespace.py
  • tests/test_utils/test_plugins.py
✅ Files skipped from review due to trivial changes (1)
  • docs/contributing/extending_dascore.qmd
🚧 Files skipped from review as they are similar to previous changes (1)
  • dascore/viz/init.py

Comment thread dascore/utils/namespace.py
Comment thread dascore/utils/namespace.py Outdated
@d-chambers d-chambers added the no_ci disables CI label Mar 11, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
tests/test_utils/test_namespace.py (2)

58-62: Use direct attribute access instead of getattr with a constant.

Static analysis correctly flags that getattr(inst, "bob") is equivalent to inst.bob when using a constant attribute name. Direct attribute access is cleaner and more idiomatic.

📝 Suggested fix
     def test_discoverable(self):
         """The Parent class should have the namespaces available."""
         inst = ParentClass()
-        bob = getattr(inst, "bob")
+        bob = inst.bob
         assert isinstance(bob, ParentClassNamespace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_utils/test_namespace.py` around lines 58 - 62, In
test_discoverable replace the unnecessary dynamic access getattr(inst, "bob")
with direct attribute access inst.bob: update the assertion in the
test_discoverable function to use inst.bob and keep the isinstance check against
ParentClassNamespace (inst is an instance of ParentClass and "bob" is the
namespace attribute on that class).

50-52: Remove or use the unused expected_type parameter.

The expected_type parameter is declared but never used in the method body. Either remove it or use it for its intended purpose (e.g., an assertion).

📝 Suggested fix
-    def func1(self, expected_type):
-        """First func."""
-        return self.name
+    def func1(self):
+        """First func."""
+        return self.name
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_utils/test_namespace.py` around lines 50 - 52, The method func1
declares an unused parameter expected_type; remove the parameter from func1's
signature or use it (e.g., assert isinstance(self.name, expected_type) or
perform a type check) and update any tests or callers that pass expected_type
accordingly so signatures match; refer to func1 to locate and modify the method
and any tests that call it.
dascore/utils/namespace.py (2)

51-51: Consider using a more precise type annotation.

The _registry is typed as Mapping[str, dict] but is actually a defaultdict. While this works due to structural subtyping, a more precise annotation would be dict[str, dict[str, type]] or defaultdict[str, dict[str, type]] to better reflect the actual type and the nested structure.

📝 Suggested improvement
-    _registry: ClassVar[Mapping[str, dict]] = defaultdict(dict)
+    _registry: ClassVar[defaultdict[str, dict[str, type]]] = defaultdict(dict)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dascore/utils/namespace.py` at line 51, The _registry ClassVar annotation is
too generic; change the type of _registry to precisely reflect its runtime
structure (e.g. dict[str, dict[str, type]] or collections.defaultdict[str,
dict[str, type]]), update the ClassVar[_registry] declaration to use that
precise type, and adjust imports if needed (e.g. import defaultdict or
typing.Dict/Type) so the declaration for _registry in namespace.py matches the
actual defaultdict of nested dicts.

103-118: Namespace instances are created on every attribute access.

Each access to a namespace attribute (e.g., patch.viz) creates a new namespace instance via registry[item](self). This is likely intentional since namespaces are lightweight wrappers, but if namespace creation becomes expensive or stateful, consider caching the instance (e.g., via __dict__ assignment).

📝 Optional caching pattern
     def __getattr__(self, item):
         """Try loading a lazily registered namespace before failing."""
         # Unknown attribute; try loading the namespaces.
         maybe_load_entry_point(self._namespace_entry_point_group, item)

         # Once loaded the registry should be populated.
         registry = _MethodNameSpace._registry.get(self._namespace_entry_point_group, {})
         if item in registry:
-            return registry[item](self)
+            ns = registry[item](self)
+            # Cache for subsequent accesses
+            object.__setattr__(self, item, ns)
+            return ns

         # If that fails, see if there is anything specific for this name to raise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dascore/utils/namespace.py` around lines 103 - 118, Namespace attribute
access in __getattr__ repeatedly creates new namespace instances; modify
__getattr__ (in class _MethodNameSpace) to cache the created instance from
registry[item](self) onto the object (e.g., assign to self.__dict__[item] or
setattr(self, item, instance)) before returning it so subsequent accesses return
the same instance; keep using _MethodNameSpace._registry,
_namespace_entry_point_group, and _namespace_attr_errors for lookup and error
handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@dascore/utils/namespace.py`:
- Line 51: The _registry ClassVar annotation is too generic; change the type of
_registry to precisely reflect its runtime structure (e.g. dict[str, dict[str,
type]] or collections.defaultdict[str, dict[str, type]]), update the
ClassVar[_registry] declaration to use that precise type, and adjust imports if
needed (e.g. import defaultdict or typing.Dict/Type) so the declaration for
_registry in namespace.py matches the actual defaultdict of nested dicts.
- Around line 103-118: Namespace attribute access in __getattr__ repeatedly
creates new namespace instances; modify __getattr__ (in class _MethodNameSpace)
to cache the created instance from registry[item](self) onto the object (e.g.,
assign to self.__dict__[item] or setattr(self, item, instance)) before returning
it so subsequent accesses return the same instance; keep using
_MethodNameSpace._registry, _namespace_entry_point_group, and
_namespace_attr_errors for lookup and error handling.

In `@tests/test_utils/test_namespace.py`:
- Around line 58-62: In test_discoverable replace the unnecessary dynamic access
getattr(inst, "bob") with direct attribute access inst.bob: update the assertion
in the test_discoverable function to use inst.bob and keep the isinstance check
against ParentClassNamespace (inst is an instance of ParentClass and "bob" is
the namespace attribute on that class).
- Around line 50-52: The method func1 declares an unused parameter
expected_type; remove the parameter from func1's signature or use it (e.g.,
assert isinstance(self.name, expected_type) or perform a type check) and update
any tests or callers that pass expected_type accordingly so signatures match;
refer to func1 to locate and modify the method and any tests that call it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ed820cf6-138f-4576-bd37-afd7305ee2a9

📥 Commits

Reviewing files that changed from the base of the PR and between cece2ba and 10e8ada.

📒 Files selected for processing (2)
  • dascore/utils/namespace.py
  • tests/test_utils/test_namespace.py

@d-chambers d-chambers removed the no_ci disables CI label Mar 11, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@dascore/utils/plugins.py`:
- Around line 10-13: get_entry_point_loaders currently silently drops duplicate
entry-point names; change it to detect duplicates in the list returned by
entry_points(group=entry_point_group) and issue a UserWarning while still
letting the last-one-wins behavior remain (to match the pattern in
dascore/utils/namespace.py). Implement this by iterating the entry points,
tracking seen names, collecting duplicates, and calling warnings.warn with a
clear message that includes the duplicate names and group; then build and return
the dict {name: ep.load} so the last occurrence overrides earlier ones (keep
`@functools.cache` on get_entry_point_loaders). If you prefer the stricter
behavior instead, raise a ValueError listing the conflicting names and add a
test asserting the error on duplicate entry points (but choose one policy
consistently).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c6e77406-a0c1-48f7-8071-3f5d88786f5c

📥 Commits

Reviewing files that changed from the base of the PR and between 10e8ada and 834fc1b.

📒 Files selected for processing (1)
  • dascore/utils/plugins.py

Comment thread dascore/utils/plugins.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/test_utils/test_namespace.py`:
- Around line 92-93: Replace the bare attribute access inside the pytest.raises
block with a getattr call to avoid the B018 lint warning: inside the context
manager using pytest.raises(AttributeError, match=msg) call getattr(inst,
"not_an_attr") instead of referencing inst.not_an_attr so the AttributeError is
still raised and matched against msg; update the test in
tests/test_utils/test_namespace.py where inst and msg are used.

In `@tests/test_utils/test_plugins.py`:
- Around line 57-59: The current monkeypatch of plugin_mod.entry_points uses a
lambda that ignores its group argument, so update the stub to assert the
requested entry-point group before returning entry_point_list; replace the
lambda with a small function (or lambda that asserts) that accepts the group
keyword arg and raises/asserts if group != expected_group (the group string used
by get_entry_point_loaders()), ensuring get_entry_point_loaders() is querying
the correct group and resolving the ARG005 warning.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3ac404e0-e40e-4709-8864-3b6d87ea6cc4

📥 Commits

Reviewing files that changed from the base of the PR and between 834fc1b and fdcae7c.

📒 Files selected for processing (5)
  • dascore/utils/namespace.py
  • dascore/utils/plugins.py
  • docs/contributing/extending_dascore.qmd
  • tests/test_utils/test_namespace.py
  • tests/test_utils/test_plugins.py
✅ Files skipped from review due to trivial changes (1)
  • docs/contributing/extending_dascore.qmd
🚧 Files skipped from review as they are similar to previous changes (1)
  • dascore/utils/namespace.py

Comment on lines +92 to +93
with pytest.raises(AttributeError, match=msg):
inst.not_an_attr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use getattr here to avoid the B018 lint failure.

The bare inst.not_an_attr expression works for the exception check, but Ruff flags it as a useless expression.

Suggested change
         with pytest.raises(AttributeError, match=msg):
-            inst.not_an_attr
+            getattr(inst, "not_an_attr")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
with pytest.raises(AttributeError, match=msg):
inst.not_an_attr
with pytest.raises(AttributeError, match=msg):
getattr(inst, "not_an_attr")
🧰 Tools
🪛 Ruff (0.15.5)

[warning] 93-93: Found useless expression. Either assign it to a variable or remove it.

(B018)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_utils/test_namespace.py` around lines 92 - 93, Replace the bare
attribute access inside the pytest.raises block with a getattr call to avoid the
B018 lint warning: inside the context manager using
pytest.raises(AttributeError, match=msg) call getattr(inst, "not_an_attr")
instead of referencing inst.not_an_attr so the AttributeError is still raised
and matched against msg; update the test in tests/test_utils/test_namespace.py
where inst and msg are used.

Comment on lines +57 to +59
monkeypatch.setattr(
plugin_mod, "entry_points", lambda *, group: entry_point_list
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Assert the requested entry-point group in the stub.

This mock currently ignores group, so the test would still pass if get_entry_point_loaders() queried the wrong group. It also leaves the Ruff ARG005 warning unresolved.

Suggested change
-        monkeypatch.setattr(
-            plugin_mod, "entry_points", lambda *, group: entry_point_list
-        )
+        def fake_entry_points(*, group):
+            assert group == entry_point_group
+            return entry_point_list
+
+        monkeypatch.setattr(plugin_mod, "entry_points", fake_entry_points)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
monkeypatch.setattr(
plugin_mod, "entry_points", lambda *, group: entry_point_list
)
def fake_entry_points(*, group):
assert group == entry_point_group
return entry_point_list
monkeypatch.setattr(plugin_mod, "entry_points", fake_entry_points)
🧰 Tools
🪛 Ruff (0.15.5)

[warning] 58-58: Unused lambda argument: group

(ARG005)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_utils/test_plugins.py` around lines 57 - 59, The current
monkeypatch of plugin_mod.entry_points uses a lambda that ignores its group
argument, so update the stub to assert the requested entry-point group before
returning entry_point_list; replace the lambda with a small function (or lambda
that asserts) that accepts the group keyword arg and raises/asserts if group !=
expected_group (the group string used by get_entry_point_loaders()), ensuring
get_entry_point_loaders() is querying the correct group and resolving the ARG005
warning.

@github-actions

Copy link
Copy Markdown

✅ Documentation built:
👉 Download
Note: You must be logged in to github and a DASDAE member to access the link.

@d-chambers
d-chambers merged commit cd3476b into master Mar 12, 2026
29 checks passed
@d-chambers
d-chambers deleted the registerable_namespace branch March 12, 2026 13:55
@coderabbitai coderabbitai Bot mentioned this pull request Mar 23, 2026
4 tasks
@coderabbitai coderabbitai Bot mentioned this pull request Apr 8, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation patch related to Patch class proc Related to processing module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant