Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Jan 26, 2026

$ python scripts/update_lib deps dis opcode 
[ ] lib: cpython/Lib/dis.py
[ ] test: cpython/Lib/test/test_dis.py (TODO: 53)

dependencies:
- [ ] dis (native: _opcode, sys)
  - [ ] collections (native: _weakref, itertools, sys)
    - [x] _collections_abc, keyword, operator, reprlib
  - [ ] io (native: _io, _thread, errno, sys)
    - [ ] codecs (native: builtins, sys)
    - [ ] os (native: os.path, sys)
      - [x] _collections_abc, abc, stat
    - [x] _collections_abc, abc, stat
  - [ ] opcode (native: _opcode, builtins)
    - [ ] _opcode_metadata
  - [x] types

dependent tests: (49 tests)
- [ ] dis: test__opcode test_ast test_code test_compile test_compiler_assemble test_dis test_dtrace test_fstring test_inspect test_opcache test_patma test_peepholer test_positional_only_arg
    - [x] inspect: test_abc test_argparse test_asyncgen test_builtin test_collections test_decimal test_enum test_functools test_generators test_grammar test_ntpath test_operator test_posixpath test_signal test_traceback test_types test_typing test_unittest test_yield_from test_zipimport
      - [ ] asyncio: test_contextlib_async test_logging test_os test_sys_settrace test_unittest
      - [ ] bdb: test_bdb
      - [ ] dataclasses: test__colorize test_genericalias test_pprint test_regrtest test_zoneinfo
      - [ ] importlib.metadata: test_importlib
      - [x] rlcompleter: test_rlcompleter
      - [ ] trace: test_trace
      - [ ] xmlrpc.server: test_docxmlrpc test_xmlrpc

[ ] lib: cpython/Lib/opcode.py
[ ] lib: cpython/Lib/_opcode_metadata.py
[x] test: cpython/Lib/test/test__opcode.py (TODO: 2)
[ ] test: cpython/Lib/test/test_opcodes.py

dependencies:
- [ ] opcode

dependent tests: (34 tests)
- [ ] opcode: test__opcode test_code test_dis test_peepholer
    - [ ] dis: test_ast test_compile test_compiler_assemble test_dtrace test_fstring test_inspect test_opcache test_patma test_positional_only_arg
      - [x] inspect: test_abc test_argparse test_asyncgen test_builtin test_collections test_decimal test_enum test_functools test_generators test_grammar test_ntpath test_operator test_posixpath test_signal test_traceback test_types test_typing test_unittest test_yield_from test_zipimport
      - [ ] trace: test_trace

Summary by CodeRabbit

  • Chores
    • Reorganized internal utility modules for improved code maintainability and reduced duplication.
    • Enhanced status reporting with synchronization markers and TODO count tracking.

✏️ Tip: You can customize this high-level summary in your review settings.

- Replace ambiguous [+] with [x] (synced) / [ ] (not synced)
- Add (TODO: n) suffix for test files with expectedFailure/skip markers
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 26, 2026

📝 Walkthrough

Walkthrough

This pull request refactors the scripts/update_lib module structure by consolidating utilities from separate modules (path.py, io_utils.py) into a new comprehensive file_utils.py, renaming command modules to use cmd_ prefixes, and updating all imports and function calls accordingly. The function test_name_from_path is renamed to get_test_module_name across the codebase.

Changes

Cohort / File(s) Summary
Module consolidation and removal
scripts/update_lib/file_utils.py
New module aggregating utilities from path.py and io_utils.py; adds 289 lines of safe file I/O, AST parsing, path utilities, comparison helpers, module/test name resolution, and CPython-to-local path conversions.
Removed utility modules
scripts/update_lib/path.py, scripts/update_lib/io_utils.py
Deleted modules; functionality moved to file_utils.py. Path parsing, lib/test conversions, module resolution, and safe I/O helpers consolidated.
Command module renames in main entry point
scripts/update_lib/__main__.py
Updates import paths from update_lib.* to update_lib.cmd_* for all subcommands (quick, copy-lib, migrate, patches, auto-mark, deps, todo); changes local alias names for auto-mark, deps, todo correspondingly.
Import path updates across cmd modules
scripts/update_lib/cmd_quick.py, scripts/update_lib/cmd_copy_lib.py, scripts/update_lib/cmd_migrate.py
Relocate imports of parse_lib_path and related utilities from update_lib.path/update_lib.io_utils to update_lib.file_utils; add imports of get_cpython_dir and remove local implementations.
Function name updates
scripts/update_lib/cmd_auto_mark.py
Replace test_name_from_path calls with get_test_module_name; update import source from update_lib.path to update_lib.file_utils.
Enhanced test/lib display logic
scripts/update_lib/cmd_deps.py
Expand imports to include count_test_todos, is_path_synced, is_test_up_to_date; update output formatting to display sync status, up-to-date status, and TODO counts for libs and tests.
Dependency utilities refactor
scripts/update_lib/deps.py
Add new parsing utilities (parse_test_imports, parse_lib_imports), TODO marker handling, test path resolution helpers (is_test_tracked, is_test_up_to_date, count_test_todos), and path synchronization logic (is_path_synced); consolidate comparison logic into compare_paths.
TODO subcommand refactor
scripts/update_lib/cmd_todo.py
Replace internal utilities with imports from update_lib.deps; remove locally-defined test-tracking/TODO-filtering functions; simplify to rely on imported modules for module listing and test status checks.
Test file updates
scripts/update_lib/tests/test_*.py
Update imports across all test files to reference new module paths (cmd_auto_mark, cmd_copy_lib, cmd_migrate, cmd_quick) and new function names (get_test_module_name, lib_to_test_path).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • moreal
  • ShaharNaveh

Poem

Hop, hop! The paths and files now unite,
From io_utils and path.py, a consolidation bright!
Into file_utils they gather with care,
Commands renamed with "cmd_" flair,
Cleaner imports, a modular delight! 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Improve deps output' is vague and generic. It uses a non-descriptive term 'Improve' without clarifying what specific improvements were made to the deps command or its output format. Consider a more specific title that captures the main change, such as 'Add sync status and TODO counts to deps output' or 'Refactor deps command with status markers and utilities extraction'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 96.88% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

Code has been automatically formatted

The code in this PR has been formatted using:

  • ruff check --select I --fix
    Please pull the latest changes before pushing again:
git pull origin deps-output

@youknowone youknowone added the skip:ci Skip running the ci label Jan 26, 2026
@youknowone youknowone marked this pull request as ready for review January 26, 2026 02:43
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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_main

Consider 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 TestTestNameFromPath but now tests get_test_module_name. Renaming to TestGetTestModuleName would 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 with pathlib.Path(), suggesting the function also accepts strings. Consider updating the type hint to pathlib.Path | str to match the actual behavior, or removing the re-wrap if only Path is 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:

Comment on lines +17 to +29
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,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +34 to +53
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

@youknowone youknowone merged commit 6a3643c into RustPython:main Jan 26, 2026
9 checks passed
@youknowone youknowone deleted the deps-output branch January 26, 2026 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip:ci Skip running the ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants