Plugin registry - #640
Conversation
Explains how to open a PR to add a row to the plugin registry CSVs so users get a helpful error message when the namespace is not installed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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:
WalkthroughNamespace attribute lookup now consults a CSV-based plugin registry and raises a new DASCorePluginError with installation guidance for registered-but-uninstalled third-party namespaces; registry loading, documentation, site nav, and tests were added or updated to support this behavior. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
added
documentation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6b646c564
ℹ️ 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".
|
|
||
| import pandas as pd | ||
|
|
||
| from dascore.exceptions import DASCorePluginError |
There was a problem hiding this comment.
This import points to a symbol that is not defined anywhere in dascore/exceptions.py, so any code path that imports dascore.utils.namespace will fail immediately with an ImportError. I checked dascore/core/patch.py:24, dascore/core/spool.py:34, dascore/io/__init__.py:17, and dascore/viz/__init__.py:5, all of which import from this module, so this would break basic Patch/Spool and built-in namespace imports in a normal install.
Useful? React with 👍 / 👎.
| f"{self.__class__.__name__} has a registered namespace of '{item}' " | ||
| f"provided by '{package_name}' but it is not installed. " | ||
| f"Install it from: {package_url}" | ||
| ) | ||
| raise DASCorePluginError(msg) |
There was a problem hiding this comment.
If a registry row is added for a namespace, this branch will raise DASCorePluginError out of __getattr__ instead of AttributeError. That changes Python's attribute protocol: callers using hasattr(obj, "ns") or getattr(obj, "ns", default) to probe optional namespaces will now get an exception for an uninstalled plugin rather than ordinary “attribute missing” behavior. The helpful install hint needs to be surfaced through an AttributeError-compatible path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dascore/utils/namespace.py (1)
41-41: Addstrict=Truetozip()calls for safety.Both nested
zip()calls lack an explicitstrict=parameter. Since the CSV columns should always have matching lengths, usingstrict=Truewill catch data corruption early.♻️ Proposed fix
- return dict(zip(df["namespace"], zip(df["package_name"], df["package_url"]))) + return dict(zip( + df["namespace"], + zip(df["package_name"], df["package_url"], strict=True), + strict=True, + ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/utils/namespace.py` at line 41, The zip calls constructing the mapping from df["namespace"] to pairs of df["package_name"], df["package_url"] should use strict=True to detect mismatched column lengths; update the inner zip that combines df["package_name"] and df["package_url"] to zip(df["package_name"], df["package_url"], strict=True) and the outer zip that pairs df["namespace"] with those pairs to zip(df["namespace"], <inner_zip>, strict=True) so any length mismatch raises immediately.
🤖 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`:
- Line 13: The import of DASCorePluginError in namespace.py is invalid because
DASCorePluginError isn't defined; update the code to either import an existing
exception (replace DASCorePluginError with DASCoreError in the import and all
usages, e.g., the reference at DASCorePluginError in namespace.py) or define the
missing class in dascore.exceptions (add class DASCorePluginError(DASCoreError):
pass) and keep the current import; ensure the symbol referenced at the failing
site matches the exception you choose.
In `@tests/test_utils/test_namespace.py`:
- Around line 267-292: The test should be updated to match the current
implementation: change the pytest.raises to expect DASCorePluginError instead of
AttributeError and update the expected message string to the implementation's
wording (e.g., "ParentClass has a registered namespace of 'cool_ns' provided
by...") so the assertion verifies the exact error type and message produced by
the __getattr__ logic (see DASCorePluginError and ParentClass/namespace
__getattr__ behavior).
---
Nitpick comments:
In `@dascore/utils/namespace.py`:
- Line 41: The zip calls constructing the mapping from df["namespace"] to pairs
of df["package_name"], df["package_url"] should use strict=True to detect
mismatched column lengths; update the inner zip that combines df["package_name"]
and df["package_url"] to zip(df["package_name"], df["package_url"], strict=True)
and the outer zip that pairs df["namespace"] with those pairs to
zip(df["namespace"], <inner_zip>, strict=True) so any length mismatch raises
immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6a0a2ab-247a-4a32-b2e3-0d7c88084451
⛔ Files ignored due to path filters (2)
dascore/plugin_registry/patch.csvis excluded by!**/*.csvdascore/plugin_registry/spool.csvis excluded by!**/*.csv
📒 Files selected for processing (6)
dascore/utils/namespace.pydocs/contributing/extending_dascore.qmddocs/index.qmddocs/supported_plugins.qmdscripts/_templates/_quarto.ymltests/test_utils/test_namespace.py
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #640 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 135 135
Lines 11605 11631 +26
=======================================
+ Hits 11597 11623 +26
Misses 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- DASCorePluginError now inherits from AttributeError (not ValueError) so hasattr() and getattr(..., default) work correctly when a namespace is registered but not installed - Add strict=True to zip() calls in _load_plugin_registry - Add test verifying hasattr returns False for uninstalled plugin namespaces Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
✅ Documentation built: |
- Add derzug (https://github.com/dasdae/derzug) to patch.csv and spool.csv - Docs table now deduplicates across CSVs and shows namespace + linked package_name in a single table instead of per-type tables - Refactor: cache-clear fixture is now class-scoped autouse in TestPluginRegistry - Remove test_multiple_rows_all_returned and strict=True from zip Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qmd now just calls get_plugin_table_str(); one test added to test_doc_utils.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 `@dascore/utils/docs.py`:
- Around line 110-117: The code assumes every CSV read into frames contains the
columns ["namespace","package_name","package_url"], causing a generic KeyError
on malformed files; before concatenating (after pd.read_csv in the frames list
or right after creating frames), validate each DataFrame from pd.read_csv and/or
the final pd.concat to ensure those exact columns exist. If any DataFrame is
missing columns, raise a clear ValueError that includes the offending file path
(from _PLUGIN_REGISTRY_DIR.glob results) and the set of missing column names;
otherwise proceed with the existing drop_duplicates/sort_values/subset
operations on the validated concatenated DataFrame. Ensure the error message
references the filename and missing columns so docs failures are actionable.
In `@tests/test_utils/test_doc_utils.py`:
- Around line 85-90: The test test_contains_registered_namespace should not rely
on live registry entries; instead create a deterministic temporary CSV with
known rows (e.g., namespace/package_name pairs), then call get_plugin_table in a
way that uses that CSV (either by refactoring get_plugin_table to accept a path
parameter or by monkeypatching the internal registry-loading helper used by
get_plugin_table), and assert the expected namespace and package_name values
from the returned DataFrame; update the test to write the temp CSV, ensure
cleanup, and use the deterministic values rather than "zug"/"derzug".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faf124dc-dece-4f86-bb55-372e7f633966
📒 Files selected for processing (4)
dascore/utils/docs.pydocs/supported_plugins.qmdtests/test_utils/test_doc_utils.pytests/test_utils/test_namespace.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/supported_plugins.qmd
| frames = [pd.read_csv(p) for p in sorted(_PLUGIN_REGISTRY_DIR.glob("*.csv"))] | ||
| if not frames: | ||
| return pd.DataFrame(columns=["namespace", "package_name", "package_url"]) | ||
| return ( | ||
| pd.concat(frames, ignore_index=True) | ||
| .drop_duplicates(subset="namespace") | ||
| .sort_values("namespace")[["namespace", "package_name", "package_url"]] | ||
| .reset_index(drop=True) |
There was a problem hiding this comment.
Handle malformed registry CSVs with explicit schema validation.
At Line 116, column selection assumes every CSV has namespace, package_name, and package_url. A malformed file currently fails with a generic KeyError, which makes docs failures harder to diagnose.
💡 Proposed hardening
def get_plugin_table() -> pd.DataFrame:
@@
- frames = [pd.read_csv(p) for p in sorted(_PLUGIN_REGISTRY_DIR.glob("*.csv"))]
+ required_cols = ["namespace", "package_name", "package_url"]
+ frames = []
+ for path in sorted(_PLUGIN_REGISTRY_DIR.glob("*.csv")):
+ df = pd.read_csv(path)
+ missing = [col for col in required_cols if col not in df.columns]
+ if missing:
+ miss = ", ".join(missing)
+ raise ValueError(
+ f"Plugin registry CSV '{path}' is missing required column(s): {miss}"
+ )
+ frames.append(df)
if not frames:
- return pd.DataFrame(columns=["namespace", "package_name", "package_url"])
+ return pd.DataFrame(columns=required_cols)
return (
pd.concat(frames, ignore_index=True)
.drop_duplicates(subset="namespace")
- .sort_values("namespace")[["namespace", "package_name", "package_url"]]
+ .sort_values("namespace")[required_cols]
.reset_index(drop=True)
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dascore/utils/docs.py` around lines 110 - 117, The code assumes every CSV
read into frames contains the columns
["namespace","package_name","package_url"], causing a generic KeyError on
malformed files; before concatenating (after pd.read_csv in the frames list or
right after creating frames), validate each DataFrame from pd.read_csv and/or
the final pd.concat to ensure those exact columns exist. If any DataFrame is
missing columns, raise a clear ValueError that includes the offending file path
(from _PLUGIN_REGISTRY_DIR.glob results) and the set of missing column names;
otherwise proceed with the existing drop_duplicates/sort_values/subset
operations on the validated concatenated DataFrame. Ensure the error message
references the filename and missing columns so docs failures are actionable.
| def test_contains_registered_namespace(self): | ||
| """Registered namespaces should appear in the returned DataFrame.""" | ||
| df = get_plugin_table() | ||
| assert "zug" in df["namespace"].values | ||
| assert "derzug" in df["package_name"].values | ||
|
|
There was a problem hiding this comment.
Avoid hard-coding live registry entries in this unit test.
This test is tied to specific repository data ("zug", "derzug"), so unrelated registry edits can break it. Prefer building a temporary CSV and asserting deterministic output.
💡 Proposed deterministic test rewrite
- def test_contains_registered_namespace(self):
+ def test_contains_registered_namespace(self, monkeypatch, tmp_path):
"""Registered namespaces should appear in the returned DataFrame."""
+ import dascore.utils.namespace as ns_module
+
+ pd.DataFrame(
+ {
+ "namespace": ["cool_ns"],
+ "package_name": ["coolpkg"],
+ "package_url": ["https://example.com/coolpkg"],
+ }
+ ).to_csv(tmp_path / "patch.csv", index=False)
+ monkeypatch.setattr(ns_module, "_PLUGIN_REGISTRY_DIR", tmp_path)
+
df = get_plugin_table()
- assert "zug" in df["namespace"].values
- assert "derzug" in df["package_name"].values
+ assert "cool_ns" in df["namespace"].values
+ assert "coolpkg" in df["package_name"].values🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_utils/test_doc_utils.py` around lines 85 - 90, The test
test_contains_registered_namespace should not rely on live registry entries;
instead create a deterministic temporary CSV with known rows (e.g.,
namespace/package_name pairs), then call get_plugin_table in a way that uses
that CSV (either by refactoring get_plugin_table to accept a path parameter or
by monkeypatching the internal registry-loading helper used by
get_plugin_table), and assert the expected namespace and package_name values
from the returned DataFrame; update the test to write the temp CSV, ensure
cleanup, and use the deterministic values rather than "zug"/"derzug".
|
I decided to put this on the dev branch rather than master. |
1 participant
Description
This PR creates a simple plugin registry on the Patch and Spool, and updates docs accordingly.
Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Documentation
Tests