Refactor get_metadata to return None when create=False#49
Refactor get_metadata to return None when create=False#49AlanRockefeller merged 2 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughSidecar metadata handling was made defensive: Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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 |
Previously, get_metadata with create=False returned a blank EntryMetadata object if no entry existed. This obscured the difference between missing metadata and empty metadata. This commit updates the API to return None when a requested sidecar entry is missing and create=False. Changes: - Added overload type hints to get_metadata for strict return typing. - Updated read-only metadata lookups in AppController to handle None. - Updated side_effect mocks in tests to accept keyword arguments.
![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)
faststack/app.py (1)
2118-2133: Consider centralizing the default metadata shape.The fallback flag/date values are now duplicated across
_get_metadata_dict()andget_current_metadata(). A small shared helper would make future metadata fields less likely to drift between grid and loupe views.Also applies to: 2384-2394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 2118 - 2133, Create a single source of truth for the default metadata shape and replace the duplicated literal dicts in _get_metadata_dict() and get_current_metadata() (and the other occurrence referenced around the second block) with a call or reference to that helper; e.g., add a constant or function like DEFAULT_METADATA_DICT() or get_default_metadata() that returns the default flag/date values, then have _get_metadata_dict() and get_current_metadata() return that default when meta is None and map meta fields from it when present so both views stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/tests/test_jump_to_last_uploaded.py`:
- Around line 69-70: The test double function side_effect used for get_metadata
must honor the create flag: update the side_effect(s) in
test_jump_to_last_uploaded.py (the mock used for get_metadata) to check
kwargs.get("create", True) and return EntryMetadata() only when create is True
and the stem is unknown; if create is False and the stem is not in
{"img1","img2","img3"} return None so the test exercises the missing-stem path
(apply same change to the other occurrence around lines 140-141).
---
Nitpick comments:
In `@faststack/app.py`:
- Around line 2118-2133: Create a single source of truth for the default
metadata shape and replace the duplicated literal dicts in _get_metadata_dict()
and get_current_metadata() (and the other occurrence referenced around the
second block) with a call or reference to that helper; e.g., add a constant or
function like DEFAULT_METADATA_DICT() or get_default_metadata() that returns the
default flag/date values, then have _get_metadata_dict() and
get_current_metadata() return that default when meta is None and map meta fields
from it when present so both views stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cb9fd8e-77d5-4d76-b40d-b5a6b7bbdc91
📒 Files selected for processing (3)
faststack/app.pyfaststack/io/sidecar.pyfaststack/tests/test_jump_to_last_uploaded.py
| def side_effect(stem, **kwargs): | ||
| return {"img1": meta1, "img2": meta2, "img3": meta3}.get(stem, EntryMetadata()) |
There was a problem hiding this comment.
Honor create=False in these get_metadata test doubles.
These mocks still fall back to EntryMetadata() for unknown stems, which preserves the old contract and can hide the regression this PR is meant to catch. If jump_to_last_uploaded() looks up a missing stem with create=False, this test would never exercise the new None path.
Suggested fix
- def side_effect(stem, **kwargs):
- return {"img1": meta1, "img2": meta2, "img3": meta3}.get(stem, EntryMetadata())
+ def side_effect(stem, **kwargs):
+ metadata = {"img1": meta1, "img2": meta2, "img3": meta3}
+ if stem in metadata:
+ return metadata[stem]
+ return EntryMetadata() if kwargs.get("create", True) else NoneAlso applies to: 140-141
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 69-69: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/tests/test_jump_to_last_uploaded.py` around lines 69 - 70, The test
double function side_effect used for get_metadata must honor the create flag:
update the side_effect(s) in test_jump_to_last_uploaded.py (the mock used for
get_metadata) to check kwargs.get("create", True) and return EntryMetadata()
only when create is True and the stem is unknown; if create is False and the
stem is not in {"img1","img2","img3"} return None so the test exercises the
missing-stem path (apply same change to the other occurrence around lines
140-141).
1 participant
Previously,
SidecarManager.get_metadata(..., create=False)returned a blankEntryMetadataobject if no entry existed. This obscured the difference between "no metadata exists" and "empty metadata exists."This commit updates the API to return
Nonewhen a requested sidecar entry is missing andcreate=False.Changes:
@overloadtype hints toYoutubeto strictly type the return value based on thecreateboolean.AppControllerto safely and explicitly handleNonevalues.side_effectmocks in tests to accept**kwargs.Summary by CodeRabbit
Bug Fixes
Tests