fix: filter not wanted plugins changes#361
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughAdds GoodmapPluginBase and re-exports it from the package, switches plugin discovery to the goodmap.plugins group, collects shortcodes from ContentTransformerPluginBase instances during app setup, and includes a unit test and platzky version bump. ChangesPlugin System Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit_tests/test_goodmap.py (1)
166-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate manifest module assertion to match current plugin contract.
This test still expects
"./Button", but the app now emits"./Plugin", so the assertion is stale.Suggested fix
- "module": "./Button", + "module": "./Plugin",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_goodmap.py` around lines 166 - 171, The test assertion for app.config["PLUGIN_MANIFEST"] is stale: update the expected manifest entry's "module" value from "./Button" to the current contract "./Plugin" so the assertion matches the emitted manifest; locate the assertion in tests/unit_tests/test_goodmap.py that compares app.config["PLUGIN_MANIFEST"] and change the module field in that expected dict to "./Plugin".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit_tests/test_goodmap.py`:
- Around line 166-171: The test assertion for app.config["PLUGIN_MANIFEST"] is
stale: update the expected manifest entry's "module" value from "./Button" to
the current contract "./Plugin" so the assertion matches the emitted manifest;
locate the assertion in tests/unit_tests/test_goodmap.py that compares
app.config["PLUGIN_MANIFEST"] and change the module field in that expected dict
to "./Plugin".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31467e5f-8bd2-402a-bf11-5bb52e1e49e0
📒 Files selected for processing (4)
goodmap/__init__.pygoodmap/goodmap.pygoodmap/plugin.pytests/unit_tests/test_goodmap.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit_tests/test_goodmap.py (1)
166-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate stale manifest module expectation to match runtime output.
Line 170 still asserts
"./Button", butgoodmap/goodmap.pynow emits"./Plugin"at Line 70. This makes the test contract inconsistent with current behavior.Suggested patch
assert app.config["PLUGIN_MANIFEST"] == [ { "scope": "my_plugin", "url": "/plugins/my_plugin/static/remoteEntry.js", - "module": "./Button", + "module": "./Plugin", } ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_goodmap.py` around lines 166 - 171, The test asserts app.config["PLUGIN_MANIFEST"] contains a module "./Button" but the runtime (goodmap.py) now emits "./Plugin"; update the expected manifest entry in the test assertion to use "module": "./Plugin" so the test matches actual output (locate the assertion comparing app.config["PLUGIN_MANIFEST"] in the test and replace "./Button" with "./Plugin").
🧹 Nitpick comments (1)
tests/unit_tests/test_goodmap.py (1)
294-307: ⚡ Quick winStrengthen regression coverage by making one plugin explicitly non-field.
The new test currently gives both plugins
ALL_CONTENT_TYPES, so it won’t fail if field-only filtering is accidentally reintroduced later. Make one plugin omit"field"to assert the intended behavior change.Suggested patch
{ "name": "post_plugin", "config": {}, - "allowed_content_types": list(ALL_CONTENT_TYPES), + "allowed_content_types": [ + ct for ct in ALL_CONTENT_TYPES if ct != "field" + ], "allowed_topics": ["general", "content", "security"], },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_goodmap.py` around lines 294 - 307, The test currently gives both plugins ALL_CONTENT_TYPES; update the plugin definitions created via _make_test_app_config so one plugin is explicitly non-field by removing "field" from its allowed_content_types (e.g., keep "field_plugin" as-is and change "post_plugin" to use allowed_content_types that exclude "field", e.g. list(ALL_CONTENT_TYPES - {"field"}) or an explicit list without "field") so the test will catch regressions that reintroduce field-only filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit_tests/test_goodmap.py`:
- Around line 166-171: The test asserts app.config["PLUGIN_MANIFEST"] contains a
module "./Button" but the runtime (goodmap.py) now emits "./Plugin"; update the
expected manifest entry in the test assertion to use "module": "./Plugin" so the
test matches actual output (locate the assertion comparing
app.config["PLUGIN_MANIFEST"] in the test and replace "./Button" with
"./Plugin").
---
Nitpick comments:
In `@tests/unit_tests/test_goodmap.py`:
- Around line 294-307: The test currently gives both plugins ALL_CONTENT_TYPES;
update the plugin definitions created via _make_test_app_config so one plugin is
explicitly non-field by removing "field" from its allowed_content_types (e.g.,
keep "field_plugin" as-is and change "post_plugin" to use allowed_content_types
that exclude "field", e.g. list(ALL_CONTENT_TYPES - {"field"}) or an explicit
list without "field") so the test will catch regressions that reintroduce
field-only filtering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3896c2d-503c-464b-a789-aca8da6cfe71
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
goodmap/goodmap.pypyproject.tomltests/unit_tests/test_goodmap.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🧪 E2E Test Results📊 View full workflow run
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
goodmap/goodmap.py (1)
30-30:⚠️ Potential issue | 🟠 MajorUpdate documentation to reflect plugin entry-point group change from
platzky.pluginstogoodmap.plugins.The code now uses
goodmap.pluginsas the plugin discovery group, butdocs/plugins.rststill references the oldplatzky.pluginsgroup. This inconsistency will mislead plugin developers. Update the documentation to consistently refer togoodmap.pluginsthroughout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@goodmap/goodmap.py` at line 30, The _PLUGIN_ENTRY_POINT_GROUP constant in goodmap.py has been updated to use "goodmap.plugins", but the documentation in docs/plugins.rst still references the old "platzky.plugins" entry point group. Update all references to the plugin entry point group in docs/plugins.rst to use "goodmap.plugins" instead of "platzky.plugins" to maintain consistency with the code change and prevent misleading plugin developers.
🧹 Nitpick comments (1)
goodmap/goodmap.py (1)
171-171: 💤 Low valueTighten the type annotation for
shortcodes.The type annotation
dict[str, Any]is too broad. Based on the test context showingshortcodes: ClassVar[dict[str, Shortcode]], a more precise type would improve type safety.♻️ Proposed fix to use a more specific type
+from platzky.shortcodes import Shortcode + - shortcodes: dict[str, Any] = {} + shortcodes: dict[str, Shortcode] = {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@goodmap/goodmap.py` at line 171, The shortcodes variable is currently typed as dict[str, Any] which is too broad and reduces type safety. Change the type annotation for the shortcodes variable to dict[str, Shortcode] to provide a more precise type that accurately reflects the values stored in the dictionary, based on the test context showing the expected structure of this class variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@goodmap/goodmap.py`:
- Around line 171-174: The shortcodes.update call in the loop iterating over
app.loaded_plugins silently overwrites duplicate shortcode names without any
indication. Before calling shortcodes.update for each
ContentTransformerPluginBase plugin, check if any shortcode names already exist
in the shortcodes dictionary by comparing the keys of shortcodes with the keys
of plugin.shortcodes. When a collision is detected, log a warning message that
identifies the conflicting shortcode names and which plugin is overwriting them,
then proceed with the update. This ensures visibility into shortcode conflicts
while maintaining the current behavior of allowing later plugins to override
earlier ones.
---
Outside diff comments:
In `@goodmap/goodmap.py`:
- Line 30: The _PLUGIN_ENTRY_POINT_GROUP constant in goodmap.py has been updated
to use "goodmap.plugins", but the documentation in docs/plugins.rst still
references the old "platzky.plugins" entry point group. Update all references to
the plugin entry point group in docs/plugins.rst to use "goodmap.plugins"
instead of "platzky.plugins" to maintain consistency with the code change and
prevent misleading plugin developers.
---
Nitpick comments:
In `@goodmap/goodmap.py`:
- Line 171: The shortcodes variable is currently typed as dict[str, Any] which
is too broad and reduces type safety. Change the type annotation for the
shortcodes variable to dict[str, Shortcode] to provide a more precise type that
accurately reflects the values stored in the dictionary, based on the test
context showing the expected structure of this class variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
🧪 E2E Test Results📊 View full workflow run
|
|
🧪 E2E Test Results📊 View full workflow run
|
1 participant
Summary by CodeRabbit
GoodmapPluginBaseat the package top level.platzkydependency version.