[update_lib] fast date lookup for todo#7299
Merged
youknowone merged 3 commits intoRustPython:mainfrom Mar 1, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a bulk git date-caching lookup in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as cmd_todo.py
participant Deps as deps.py
participant Cache as BulkCache
participant Git as git CLI
CLI->>Deps: request last_updated for module/test paths
Deps->>Cache: check bulk cache for Lib-relative paths
alt cache miss
Deps->>Git: single bulk git query for all Lib/ paths
Git-->>Deps: per-file commit dates + directory rollups
Deps->>Cache: populate bulk cache (path -> date, dir aggregates)
end
Deps->>Cache: resolve most recent date for requested paths
Cache-->>Deps: return resolved date or None
Deps-->>CLI: return last_updated value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/update_lib/deps.py`:
- Around line 1164-1183: _normalize path strings before comparing to avoid false
negatives: in _lookup_last_updated(), call pathlib.Path(lib_prefix).as_posix()
and pass that normalized string into _lib_prefix_stripped (or normalize inside
_lib_prefix_stripped), and for each input path p convert it to a POSIX string
via pathlib.Path(p).as_posix() before using startswith() and slicing; keep using
_bulk_last_updated() and the existing key lookup but ensure both prefix and p
are normalized to POSIX so Windows backslashes and './' variants won't cause
cache misses.
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin update-lib |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/update_lib/deps.py (1)
1123-1129: Consider specifyingcwdfor the git command to improve robustness.The git command runs without an explicit
cwd, assuming the script is invoked from the repository root. If the script is run from a different working directory, theLib/path may not resolve correctly.💡 Suggested improvement
+ # Run from repo root (parent of scripts/) + repo_root = pathlib.Path(__file__).parent.parent.parent try: result = subprocess.run( ["git", "log", "--format=%cd", "--date=short", "--name-only", "--", "Lib/"], capture_output=True, text=True, timeout=30, + cwd=repo_root, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/update_lib/deps.py` around lines 1123 - 1129, The subprocess.run call that executes git (the invocation assigning to result) should set an explicit cwd so the "Lib/" path is resolved relative to the repository root rather than the caller's current directory; update the subprocess.run call to pass cwd pointing to the repo root (resolve via the script's file location using pathlib.Path(__file__).resolve().parents[...] or similar) and ensure pathlib/os is imported if not already.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/update_lib/deps.py`:
- Around line 1123-1129: The subprocess.run call that executes git (the
invocation assigning to result) should set an explicit cwd so the "Lib/" path is
resolved relative to the repository root rather than the caller's current
directory; update the subprocess.run call to pass cwd pointing to the repo root
(resolve via the script's file location using
pathlib.Path(__file__).resolve().parents[...] or similar) and ensure pathlib/os
is imported if not already.
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
1 participant
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 by CodeRabbit
Refactor
Bug Fixes
Chores