feat: enforce keys in sources Base class#231
Conversation
7d9615e to
48b457c
Compare
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthrough
ChangesBase source metadata validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bc49138 to
c3a7f88
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tests/test_base_source.py (1)
132-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutable default class attributes flagged by Ruff (RUF012).
nullified_questions = []andrequired_metadata_keys = {"ticker_renames"}are unannotated mutable class attribute defaults. Ruff's rule notes this can be resolved by explicitly annotating the variable with typing.ClassVar to indicate that it is intended to be shared across all instances, or using an immutable data type (e.g. a tuple instead of a list) for the default value. Since these test stubs won't be instantiated multiple times, the risk is negligible, but annotating (or usingfrozenset/tuple) would silence the lint warning consistently with how the base class likely declares these asClassVar.🧹 Example fix using ClassVar annotations
- class _BadSpecific(BaseSource): - name = "fake_missing_specific" - source_type = SourceType.DATASET - required_metadata_keys = {"ticker_renames"} + class _BadSpecific(BaseSource): + name = "fake_missing_specific" + source_type = SourceType.DATASET + required_metadata_keys: ClassVar[set[str]] = {"ticker_renames"}Also applies to: 155-180
🤖 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 `@src/tests/test_base_source.py` around lines 132 - 154, The test stub in test_missing_source_specific_required_key_raises uses mutable class defaults that Ruff flags as RUF012. Update the inline _BadSpecific class inside the test to make the shared intent explicit by annotating nullified_questions and required_metadata_keys as ClassVar, or switch the defaults to immutable types like tuple/frozenset. Keep the existing BaseSource and SOURCE_METADATA setup unchanged; only adjust the class attribute declarations in the test stubs that mirror BaseSource metadata.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@src/tests/test_base_source.py`:
- Around line 132-154: The test stub in
test_missing_source_specific_required_key_raises uses mutable class defaults
that Ruff flags as RUF012. Update the inline _BadSpecific class inside the test
to make the shared intent explicit by annotating nullified_questions and
required_metadata_keys as ClassVar, or switch the defaults to immutable types
like tuple/frozenset. Keep the existing BaseSource and SOURCE_METADATA setup
unchanged; only adjust the class attribute declarations in the test stubs that
mirror BaseSource metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f37b6e3a-8da3-46fb-98a8-3edc896a050f
📒 Files selected for processing (5)
src/sources/_base.pysrc/sources/_metadata.pysrc/sources/fred.pysrc/sources/yfinance.pysrc/tests/test_base_source.py
💤 Files with no reviewable changes (1)
- src/sources/fred.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/sources/yfinance.py
- src/sources/_metadata.py
- src/sources/_base.py
houtanb
left a comment
houtanb
left a comment
There was a problem hiding this comment.
small rename for clarity.
Also, commit message could be more clear; it looks like it was truncated relative to the PR title which is better
while `_allow_missing_metadata` might seem a tad awkward, it actually helps preserve both tests as well as the ability to validate that a source's name value (e.g. in `fred.py`) actually exists in `_metadata.py`
c3a7f88 to
d9a635f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tests/test_base_source.py (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate intentional mutable class attributes with
ClassVar.Ruff reports RUF012 for these shared class-level list/set values. Add the appropriate
ClassVar[...]annotations, or use immutable values where the base-class contract permits it.Also applies to: 150-150, 173-173
🤖 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 `@src/tests/test_base_source.py` at line 24, Ruff RUF012 flags the shared mutable class-level collections in the affected test base classes. Import ClassVar from typing and annotate each referenced class attribute, including nullified_questions and the attributes at the other reported locations, with the appropriate ClassVar[list[...]] or ClassVar[set[...]] type; use immutable values only where compatible with the base-class contract.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@src/tests/test_base_source.py`:
- Line 24: Ruff RUF012 flags the shared mutable class-level collections in the
affected test base classes. Import ClassVar from typing and annotate each
referenced class attribute, including nullified_questions and the attributes at
the other reported locations, with the appropriate ClassVar[list[...]] or
ClassVar[set[...]] type; use immutable values only where compatible with the
base-class contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d55766d4-644e-466c-8f0c-bbec390d32bb
📒 Files selected for processing (5)
src/sources/_base.pysrc/sources/_metadata.pysrc/sources/fred.pysrc/sources/yfinance.pysrc/tests/test_base_source.py
💤 Files with no reviewable changes (1)
- src/sources/fred.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sources/_metadata.py
2 participants
while
_allow_missing_metadatamight seem a tad awkward, it actually helps preserve both tests as well as the ability to validate that a source's name value (e.g. in fred.py) actually exists in _metadata.pySolves #205
Full pipeline test via the harness - all jobs deploy, run and achieve expected parity.
Summary by CodeRabbit
nullified_questionsplus any source-specific required keys (e.g.,ticker_renames).nullified_questions.nullified_questionsas part of its class metadata.