Encapsulate WG21Index.papers behind snapshot and count accessors#92
Encapsulate WG21Index.papers behind snapshot and count accessors#92henry0816191 wants to merge 6 commits into
Conversation
Privatize the index map to _papers, add paper_count() and a test seam, migrate cross-thread readers and tests, and update concurrency docs.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWG21Index now keeps paper data in a private ChangesWG21Index Encapsulation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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)
src/paperscout/monitor.py (1)
372-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
paper_count()call.
wg21.paper_count()is called twice: once to computepaper_count(line 374) and again at line 379 for the log message, even thoughpaper_count == wg21.paper_count()wheneverwg21 is not None. Reuse the already-computed value.♻️ Proposed simplification
if self.cfg.enable_bulk_wg21 and wg21 is not None: - log.info("SEED wg21.link loaded papers=%d", wg21.paper_count()) + log.info("SEED wg21.link loaded papers=%d", paper_count)🤖 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/paperscout/monitor.py` around lines 372 - 379, Reuse the already computed paper_count in the wg21 branch of monitor.py instead of calling wg21.paper_count() again in the log message. Update the logic around self._wg21_index() so the SEED wg21.link loaded log uses paper_count directly when wg21 is not None, keeping the existing fallback behavior unchanged.
🤖 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 `@src/paperscout/sources.py`:
- Line 77: The refresh methods are returning the live backing map via
IndexRefreshResult, so callers can mutate internal state through result.papers.
Update the return sites in the refresh paths that use _commit_index() to pass a
copied snapshot of self._papers instead of the original dict, preserving the
snapshot contract while keeping IndexRefreshResult and the related accessors
unchanged.
---
Nitpick comments:
In `@src/paperscout/monitor.py`:
- Around line 372-379: Reuse the already computed paper_count in the wg21 branch
of monitor.py instead of calling wg21.paper_count() again in the log message.
Update the logic around self._wg21_index() so the SEED wg21.link loaded log uses
paper_count directly when wg21 is not None, keeping the existing fallback
behavior unchanged.
🪄 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: b702d900-5981-4839-8632-58dc03e88e02
📒 Files selected for processing (9)
CONTRIBUTING.mddocs/architecture.mdsrc/paperscout/__main__.pysrc/paperscout/concurrency.pysrc/paperscout/monitor.pysrc/paperscout/sources.pytests/test_callback_protocols.pytests/test_monitor.pytests/test_sources.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)
src/paperscout/sources.py (1)
94-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
stale=Truefor the persisted-cache fallback
This branch runs after a live download failure, so returningIndexRefreshResult(..., stale=False)makes degraded data look fresh and suppresses the stale-path handling insrc/paperscout/monitor.py. The fallback should keepstale=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 `@src/paperscout/sources.py` around lines 94 - 103, The persisted-cache fallback in the index refresh flow is incorrectly marking degraded data as fresh. In the branch inside the refresh logic where `self._cache.read()` succeeds and `_parse_raw`, `_commit_index`, and the `INDEX-STALE-FALLBACK` warning are used, change the returned `IndexRefreshResult` to preserve the stale state so downstream handling in `monitor.py` still treats it as stale.
🤖 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 `@src/paperscout/sources.py`:
- Around line 94-103: The persisted-cache fallback in the index refresh flow is
incorrectly marking degraded data as fresh. In the branch inside the refresh
logic where `self._cache.read()` succeeds and `_parse_raw`, `_commit_index`, and
the `INDEX-STALE-FALLBACK` warning are used, change the returned
`IndexRefreshResult` to preserve the stale state so downstream handling in
`monitor.py` still treats it as stale.
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)
src/paperscout/sources.py (1)
94-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn
stale=Truewhen falling back to persisted cache
src/paperscout/sources.py:94-103— this branch runs only after the download fails and the code reuses persisted cache, sostale=Falsehides degraded data frommonitor._poll_sources(), which keys_index_staleand theINDEX-STALEwarning offrefresh_result.stale.Proposed fix
- return IndexRefreshResult(dict(self._papers), stale=False) + return IndexRefreshResult(dict(self._papers), stale=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 `@src/paperscout/sources.py` around lines 94 - 103, The fallback branch in `PaperIndexSource` currently returns `IndexRefreshResult(..., stale=False)` even though it is reusing persisted cache after a failed download. Update the stale-cache fallback path in `sources.py` so the `IndexRefreshResult` returned from this branch is marked `stale=True`, keeping `monitor._poll_sources()` and the `INDEX-STALE` warning aligned with degraded data. Use the existing `stale` handling around `_cache.read()`, `_parse_raw()`, and `_commit_index()` to locate the branch and change only the returned refresh result state.
🤖 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 `@src/paperscout/sources.py`:
- Around line 94-103: The fallback branch in `PaperIndexSource` currently
returns `IndexRefreshResult(..., stale=False)` even though it is reusing
persisted cache after a failed download. Update the stale-cache fallback path in
`sources.py` so the `IndexRefreshResult` returned from this branch is marked
`stale=True`, keeping `monitor._poll_sources()` and the `INDEX-STALE` warning
aligned with degraded data. Use the existing `stale` handling around
`_cache.read()`, `_parse_raw()`, and `_commit_index()` to locate the branch and
change only the returned refresh result state.
|
@coderabbitai full review again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_callback_protocols.py (1)
46-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWire
paper_counton the mock for consistency.
wg21is aMagicMock(spec=WG21Index)with_papersset directly, butpaper_countisn't wired, sowg21.paper_count()returns a genericMagicMock, not a real count derived from_papers.Scheduler.seed()uses this value in a%d-formatted log message. Consider mirroring the pattern used intests/test_monitor.py::_make_mock_wg21, which wiresmock.paper_count = lambda: len(mock._papers).🔧 Suggested fix
wg21 = MagicMock(spec=WG21Index) wg21.source_id = SOURCE_WG21_INDEX wg21.fetch = AsyncMock(return_value=IndexRefreshResult({}, stale=False)) wg21.diff = MagicMock(return_value=DiffResult([], [])) + wg21._papers = {} + wg21.paper_count = lambda: len(wg21._papers)🤖 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/test_callback_protocols.py` around lines 46 - 69, Wire the WG21Index mock in this test so Scheduler.seed() sees a real paper count instead of a generic MagicMock: in the setup for wg21, keep the existing _papers assignment but also make paper_count return the length of _papers, matching the pattern used by _make_mock_wg21 in tests/test_monitor.py. This will ensure the %d log path in Scheduler and poll_once() behave consistently with the mock state.
🤖 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 `@src/paperscout/sources.py`:
- Around line 195-202: The atomic publish in the index commit path is reusing
the caller-owned papers dict as the live backing store, which allows later
external mutation to affect the index. In the commit/publish flow around the
atomic swap in the paper index class methods such as _commit_index(),
_parse_and_index(), and the _papers assignment, create owned copies of the maps
before storing them so the published snapshot is detached from caller state
while preserving the same atomic reference swap behavior.
---
Nitpick comments:
In `@tests/test_callback_protocols.py`:
- Around line 46-69: Wire the WG21Index mock in this test so Scheduler.seed()
sees a real paper count instead of a generic MagicMock: in the setup for wg21,
keep the existing _papers assignment but also make paper_count return the length
of _papers, matching the pattern used by _make_mock_wg21 in
tests/test_monitor.py. This will ensure the %d log path in Scheduler and
poll_once() behave consistently with the mock state.
🪄 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: e9f7a4bb-76fa-4f75-a140-cf4757ae30ae
📒 Files selected for processing (9)
CONTRIBUTING.mddocs/architecture.mdsrc/paperscout/__main__.pysrc/paperscout/concurrency.pysrc/paperscout/monitor.pysrc/paperscout/sources.pytests/test_callback_protocols.pytests/test_monitor.pytests/test_sources.py
1 participant
Changes
src/paperscout/sources.pyself.papers→self._papers(private backing store)paper_count(),_replace_papers_for_test()_commit_index_papersProduction readers
__main__.py:paper_count_fnusesindex.paper_count()monitor.py:seed()useswg21.paper_count()Docs
CONTRIBUTING.md,docs/architecture.md,concurrency.py: updated concurrency wordingTests
tests/test_sources.py: snapshot accessors + test seamtests/test_monitor.py:_make_mock_wg21uses_papers/paper_count; mock assigns use_paperstests/test_callback_protocols.py:wg21._papers = {}Related issue
WG21Index.papers; enforce access via existing snapshot accessors #88Summary by CodeRabbit
paper_count()accessor for WG21 index usage._papers,paper_count(), and snapshot APIs (including a test-only replacement helper).