-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Improve deps output #6874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve deps output #6874
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.