feat: enforce keys in sources Base class#231
Open
nikbpetrov wants to merge 1 commit into
Open
Conversation
nikbpetrov
force-pushed
the
metadata-keys
branch
from
7d9615e to
48b457c
Compare
Member
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughBaseSource now requires nullified_questions and subclass-specific metadata keys during class creation. Source metadata entries were updated, FredSource dropped its explicit nullified_questions attribute, YfinanceSource declared a required key, and tests now cover missing-metadata and successful-population cases. ChangesBase source metadata validation
Sequence Diagram(s)sequenceDiagram
participant Subclass as Source subclass
participant BaseSource as BaseSource.__init_subclass__
participant SOURCE_METADATA as SOURCE_METADATA
Subclass->>BaseSource: define subclass
BaseSource->>SOURCE_METADATA: look up SOURCE_METADATA[cls.name]
SOURCE_METADATA-->>BaseSource: metadata entry
BaseSource->>BaseSource: validate core keys + required_metadata_keys
BaseSource->>Subclass: setattr populated metadata fields
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 |
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`
![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
reviewed
Jul 9, 2026
houtanb
left a comment
houtanb
left a comment
Member
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
| # Source-specific metadata keys this source needs to even run, beyond the globally required set. | ||
| # Declared on the concrete subclass (e.g. yfinance -> {"ticker_renames"}) so a missing/typo'd key | ||
| # in SOURCE_METADATA fails loudly at class-definition time rather than late at runtime. | ||
| required_metadata_keys: ClassVar[set[str]] = set() |
Member
There was a problem hiding this comment.
maybe call this additional_required_metadata_keys?
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
2 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.
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 at class definition time.nullified_questions.nullified_questions, missing source-specific required keys, and successful population when requirements are met.