feat(collectors): deprecate CollectorBase in favor of AbstractCollector#219
Merged
wpak-ai merged 2 commits intoMay 18, 2026
Merged
Conversation
… update documentation
📝 WalkthroughWalkthroughCollectorBase now emits a DeprecationWarning when subclassed, with an opt-out for existing collectors; tests validate the warning and public API docs mark CollectorBase deprecated for removal in v1.0. ChangesCollectorBase Deprecation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
core/tests/test_collectors_base.py (1)
17-17: 💤 Low valueConsider using direct attribute access instead of
getattr.Since the attribute name is a constant string,
getattrprovides no additional safety over direct attribute access. Direct access would also make it clearer that the test expects the attribute to exist.♻️ Proposed refactor
- assert getattr(DjangoCommandCollector, "_skip_collector_base_deprecation") is True + assert DjangoCommandCollector._skip_collector_base_deprecation is True🤖 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 `@core/tests/test_collectors_base.py` at line 17, Replace the getattr call with direct attribute access: the test should assert the class attribute DjangoCommandCollector._skip_collector_base_deprecation is True instead of using getattr(DjangoCommandCollector, "_skip_collector_base_deprecation"); this makes the expectation explicit and clearer.
🤖 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 `@docs/Core_public_API.md`:
- Line 9: The table entry for `core.collectors.CollectorBase` contains a broken
anchor to "Migrating from `CollectorBase` to `AbstractCollector`"; add a
migration section with that exact header text so the link resolves, e.g., a
short "Migrating from `CollectorBase` to `AbstractCollector`" section describing
replacing `run()` with `name`, `validate_config()` and `collect()` (and that
`AbstractCollector.run()` wires them together), or alternatively remove the
`[Migrating from
`CollectorBase`](`#migrating-from-collectorbase-to-abstractcollector`)` reference
in the `core.collectors.CollectorBase` table entry if you prefer not to add the
guide.
---
Nitpick comments:
In `@core/tests/test_collectors_base.py`:
- Line 17: Replace the getattr call with direct attribute access: the test
should assert the class attribute
DjangoCommandCollector._skip_collector_base_deprecation is True instead of using
getattr(DjangoCommandCollector, "_skip_collector_base_deprecation"); this makes
the expectation explicit and clearer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6a7b73f-3ff5-4950-8736-daa0f6cda66a
📒 Files selected for processing (3)
core/collectors/base.pycore/tests/test_collectors_base.pydocs/Core_public_API.md
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/collectors/base.py (1)
50-51: ⚡ Quick winAttribute lookup semantics concern is valid but not applicable here.
The code uses
getattrwhich includes inherited attributes; usingcls.__dict__.getwould restrict the flag to explicitly opt-out classes only. However,DjangoCommandCollectorhas no subclasses in the codebase, so this is not a practical issue. All production collectors inherit directly fromCollectorBase, not fromDjangoCommandCollector. If future code extendsDjangoCommandCollector, the suggested change would prevent unintended warning suppression in subclasses.🤖 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 `@core/collectors/base.py` around lines 50 - 51, Replace the current attribute lookup using getattr in the CollectorBase deprecation check so that the opt-out flag only applies when a class explicitly defines it; change the condition that reads getattr(cls, "_skip_collector_base_deprecation", False) to check the class namespace directly (e.g. cls.__dict__.get("_skip_collector_base_deprecation")) so that subclasses of DjangoCommandCollector do not inherit and unintentionally suppress the warning—update the check in the method where this condition appears (referencing CollectorBase and the _skip_collector_base_deprecation symbol).
🤖 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 `@core/tests/test_collectors_base.py`:
- Line 17: The test uses getattr(DjangoCommandCollector,
"_skip_collector_base_deprecation") unnecessarily; replace the getattr call with
direct attribute access by asserting
DjangoCommandCollector._skip_collector_base_deprecation is True so the assertion
reads: assert DjangoCommandCollector._skip_collector_base_deprecation is True,
keeping the same symbol DjangoCommandCollector and attribute name
_skip_collector_base_deprecation.
---
Nitpick comments:
In `@core/collectors/base.py`:
- Around line 50-51: Replace the current attribute lookup using getattr in the
CollectorBase deprecation check so that the opt-out flag only applies when a
class explicitly defines it; change the condition that reads getattr(cls,
"_skip_collector_base_deprecation", False) to check the class namespace directly
(e.g. cls.__dict__.get("_skip_collector_base_deprecation")) so that subclasses
of DjangoCommandCollector do not inherit and unintentionally suppress the
warning—update the check in the method where this condition appears (referencing
CollectorBase and the _skip_collector_base_deprecation symbol).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55de6368-9dd9-4c00-839b-bd41cfeba457
📒 Files selected for processing (3)
core/collectors/base.pycore/tests/test_collectors_base.pydocs/Core_public_API.md
henry0816191
approved these changes
May 18, 2026
wpak-ai
approved these changes
May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DeprecationWarningwhen subclassingCollectorBase(__init_subclass__, removal planned in v1.0).DjangoCommandCollectorvia_skip_collector_base_deprecationso core glue code stays quiet.CollectorBaseas deprecated inCore_public_API.mdand point contributors toAbstractCollector.test_collector_base_subclass_emits_deprecation_warning(useswarnings.catch_warningsbecause pytest ignoresDeprecationWarningglobally).Existing collectors that subclass
CollectorBasekeep working; the warning is advisory only.Test plan
uv run pytest core/tests/test_collectors_base.py::test_collector_base_subclass_emits_deprecation_warning -quv run pytest core/tests/test_collectors_base.py -q(with DB available for@pytest.mark.django_dbtests)Closes #212
Summary by CodeRabbit
Deprecated
Documentation
Tests