Conversation
- Replace ambiguous [+] with [x] (synced) / [ ] (not synced) - Add (TODO: n) suffix for test files with expectedFailure/skip markers
📝 WalkthroughWalkthroughThis pull request refactors the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin deps-output |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/update_lib/cmd_todo.py (1)
371-379: Align TODO suffix with the stated output format.
Current output renders(<n> TODO)while the PR description shows(TODO: <n>). This is user-facing, so the format should match the example.🔧 Suggested tweak
- if todo_count > 0: - return f" ({todo_count} TODO)" + if todo_count > 0: + return f" (TODO: {todo_count})"
🤖 Fix all issues with AI agents
In `@scripts/update_lib/deps.py`:
- Around line 17-29: The import list in update_lib/file_utils currently includes
an unused symbol _dircmp_is_same which causes a Flake8 F401; remove
_dircmp_is_same from the from update_lib.file_utils import (...) group in
scripts/update_lib/deps.py so only actually used functions
(compare_dir_contents, compare_file_contents, compare_paths, construct_lib_path,
cpython_to_local_path, read_python_files, resolve_module_path,
resolve_test_path, safe_parse_ast, safe_read_text) are imported, then run the
linter to confirm the F401 is resolved.
- Around line 34-53: The helper _extract_top_level_code truncates the file at
the first def/class which causes parse_test_imports to miss valid top-level
"from test ..." imports that appear later; update parse_test_imports to run its
three regexes (_FROM_TEST_IMPORT_RE, _FROM_TEST_DOT_RE, _IMPORT_TEST_DOT_RE)
against the full file content instead of the truncated result (i.e., remove or
bypass the call to _extract_top_level_code in parse_test_imports) so the
existing multiline regexes can find all top-level imports while still relying on
their patterns to ignore indented imports.
🧹 Nitpick comments (4)
scripts/update_lib/__main__.py (1)
66-98: Import updates look good, but consider consistent alias naming.The import path updates to
cmd_*modules are correct. However, there's inconsistent alias naming:
- Lines 66, 71, 76, 81:
quick_main,copy_lib_main,migrate_main,patches_main- Lines 86, 91, 96:
cmd_auto_mark_main,cmd_deps_main,cmd_todo_mainConsider using a consistent pattern for all aliases.
♻️ Suggested consistent naming
if args.command == "auto-mark": - from update_lib.cmd_auto_mark import main as cmd_auto_mark_main + from update_lib.cmd_auto_mark import main as auto_mark_main - return cmd_auto_mark_main(remaining) + return auto_mark_main(remaining) if args.command == "deps": - from update_lib.cmd_deps import main as cmd_deps_main + from update_lib.cmd_deps import main as deps_main - return cmd_deps_main(remaining) + return deps_main(remaining) if args.command == "todo": - from update_lib.cmd_todo import main as cmd_todo_main + from update_lib.cmd_todo import main as todo_main - return cmd_todo_main(remaining) + return todo_main(remaining)scripts/update_lib/tests/test_path.py (1)
204-220: Consider renaming the test class to match the function under test.The class is still named
TestTestNameFromPathbut now testsget_test_module_name. Renaming toTestGetTestModuleNamewould improve clarity.Suggested rename
-class TestTestNameFromPath(unittest.TestCase): - """Tests for get_test_module_name function.""" +class TestGetTestModuleName(unittest.TestCase): + """Tests for get_test_module_name function."""scripts/update_lib/file_utils.py (2)
87-121: Consider extracting the shared test path resolution logic.The if/else branches duplicate the logic for extracting
lib_name, handling__init__, and the exists-check cascade. Extracting a helper could reduce duplication.Possible refactor
+def _resolve_test_path_for_name(lib_name: str, base_dir: str) -> pathlib.Path: + """Resolve test path, preferring directory over file.""" + dir_path = pathlib.Path(f"{base_dir}/Lib/test/test_{lib_name}/") + if dir_path.exists(): + return dir_path + file_path = pathlib.Path(f"{base_dir}/Lib/test/test_{lib_name}.py") + if file_path.exists(): + return file_path + return dir_path + + def lib_to_test_path(src_path: pathlib.Path) -> pathlib.Path: """...""" path_str = str(src_path).replace("\\", "/") lib_marker = "/Lib/" if lib_marker in path_str: lib_path = parse_lib_path(src_path) lib_name = lib_path.stem if lib_path.suffix == ".py" else lib_path.name if lib_name == "__init__": lib_name = lib_path.parent.name prefix = path_str[: path_str.index(lib_marker)] - dir_path = pathlib.Path(f"{prefix}/Lib/test/test_{lib_name}/") - if dir_path.exists(): - return dir_path - file_path = pathlib.Path(f"{prefix}/Lib/test/test_{lib_name}.py") - if file_path.exists(): - return file_path - return dir_path + return _resolve_test_path_for_name(lib_name, prefix) else: lib_name = src_path.stem if src_path.suffix == ".py" else src_path.name if lib_name == "__init__": lib_name = src_path.parent.name - dir_path = pathlib.Path(f"Lib/test/test_{lib_name}/") - if dir_path.exists(): - return dir_path - file_path = pathlib.Path(f"Lib/test/test_{lib_name}.py") - if file_path.exists(): - return file_path - return dir_path + return _resolve_test_path_for_name(lib_name, ".")
131-142: Type hint and implementation mismatch.The type hint specifies
pathlib.Path, but Line 139 re-wraps the argument withpathlib.Path(), suggesting the function also accepts strings. Consider updating the type hint topathlib.Path | strto match the actual behavior, or removing the re-wrap if onlyPathis expected.Option A: Update type hint to accept strings
-def get_test_module_name(test_path: pathlib.Path) -> str: +def get_test_module_name(test_path: pathlib.Path | str) -> str:
| from update_lib.file_utils import ( | ||
| _dircmp_is_same, | ||
| compare_dir_contents, | ||
| compare_file_contents, | ||
| compare_paths, | ||
| construct_lib_path, | ||
| cpython_to_local_path, | ||
| read_python_files, | ||
| resolve_module_path, | ||
| resolve_test_path, | ||
| safe_parse_ast, | ||
| safe_read_text, | ||
| ) |
There was a problem hiding this comment.
Remove unused import to avoid lint failures.
_dircmp_is_same is imported but unused (Flake8 F401).
🧹 Suggested cleanup
from update_lib.file_utils import (
- _dircmp_is_same,
compare_dir_contents,
compare_file_contents,
compare_paths,
construct_lib_path,
cpython_to_local_path,🧰 Tools
🪛 Flake8 (7.3.0)
[error] 17-17: 'update_lib.file_utils._dircmp_is_same' imported but unused
(F401)
🤖 Prompt for AI Agents
In `@scripts/update_lib/deps.py` around lines 17 - 29, The import list in
update_lib/file_utils currently includes an unused symbol _dircmp_is_same which
causes a Flake8 F401; remove _dircmp_is_same from the from update_lib.file_utils
import (...) group in scripts/update_lib/deps.py so only actually used functions
(compare_dir_contents, compare_file_contents, compare_paths, construct_lib_path,
cpython_to_local_path, read_python_files, resolve_module_path,
resolve_test_path, safe_parse_ast, safe_read_text) are imported, then run the
linter to confirm the F401 is resolved.
| def _extract_top_level_code(content: str) -> str: | ||
| """Extract only top-level code from Python content for faster parsing.""" | ||
| def_idx = content.find("\ndef ") | ||
| class_idx = content.find("\nclass ") | ||
|
|
||
| indices = [i for i in (def_idx, class_idx) if i != -1] | ||
| if indices: | ||
| content = content[: min(indices)] | ||
| return content.rstrip("\n") | ||
|
|
||
|
|
||
| _FROM_TEST_IMPORT_RE = re.compile(r"^from test import (.+)", re.MULTILINE) | ||
| _FROM_TEST_DOT_RE = re.compile(r"^from test\.(\w+)", re.MULTILINE) | ||
| _IMPORT_TEST_DOT_RE = re.compile(r"^import test\.(\w+)", re.MULTILINE) | ||
|
|
||
|
|
||
| def parse_test_imports(content: str) -> set[str]: | ||
| """Parse test file content and extract test package dependencies.""" | ||
| content = _extract_top_level_code(content) | ||
| imports = set() |
There was a problem hiding this comment.
Top‑level imports after the first def/class can be skipped.
_extract_top_level_code truncates content at the first def/class, so any later top‑level from test ... imports won’t be detected, which can undercount dependencies and TODOs. Consider scanning the full file (the regex already ignores indented imports).
🔧 Minimal fix
def parse_test_imports(content: str) -> set[str]:
"""Parse test file content and extract test package dependencies."""
- content = _extract_top_level_code(content)
imports = set()🤖 Prompt for AI Agents
In `@scripts/update_lib/deps.py` around lines 34 - 53, The helper
_extract_top_level_code truncates the file at the first def/class which causes
parse_test_imports to miss valid top-level "from test ..." imports that appear
later; update parse_test_imports to run its three regexes (_FROM_TEST_IMPORT_RE,
_FROM_TEST_DOT_RE, _IMPORT_TEST_DOT_RE) against the full file content instead of
the truncated result (i.e., remove or bypass the call to _extract_top_level_code
in parse_test_imports) so the existing multiline regexes can find all top-level
imports while still relying on their patterns to ignore indented imports.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.