Repository navigation
Match CPython group id conversion and update grp tests - #9462
Conversation
Assisted-by: OpenAI Codex: model version unavailable
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Argument conversion and lookup crates/stdlib/src/grp.rs |
getgrgid preserves the original argument for integer conversion. It raises TypeError for conversion failures, accepts -1 as a sentinel, raises OverflowError for other invalid values, and raises KeyError when lookup fails. |
Priority:
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Bug fix
Possibly related issues
- Upgrade Python libraries and test from Python 3.14 #6839 — The issue concerns updating standard-library modules and tests from CPython; the supplied context associates this change with
grpbehavior and reference tests.
Possibly related PRs
- RustPython/RustPython#9242 — Its argument-binding changes affect how surplus positional arguments are handled before conversion, including calls to
grp.getgrgid().
Suggested reviewers: youknowone, lsahn-gh
Merge Risk: 
29ced
-
No actionable merge-blocking issue remains. The argument conversion changes are mergeable subject to normal checks, including Unix group-database integration tests.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main changes: matching CPython group ID conversion behavior and updating the associated tests. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_grp.py dependencies: dependent tests: (192 tests)
[x] lib: cpython/Lib/json dependencies:
dependent tests: (21 tests)
Legend:
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
| let index = id.id.try_index(vm).map_err(|_| { | ||
| let name = id.id.class().slot_name().to_string(); | ||
| let name = &name[..name.floor_char_boundary(200.min(name.len()))]; | ||
| vm.new_type_error(format!("gid should be integer, not {name}")) |
There was a problem hiding this comment.
we must be able to override this using ArgIndex now. pls check other error messages if this is necessary.
There was a problem hiding this comment.
Switched to the existing ArgIndex::try_from_object path in 29ced2896d, without adding a new converter API.
The call-site error override is still needed: CPython's _Py_Gid_Converter deliberately replaces every _PyNumber_Index failure, including exceptions raised by __index__, with TypeError("gid should be integer, not <type>"). The current #[pyarg(error_msg = ...)] supports only a fixed string, so it cannot preserve this type-dependent diagnostic. Both overflow messages and the normalized-gid KeyError were also checked against that implementation and remain unchanged.
Scoped Cargo check and strict Clippy passed, along with normal commit hooks. NSS was not invoked locally; the canonical test_grp coverage is left to the new-head CI.
Preserve CPython-specific gid index failure diagnostics through the existing argument converter. Assisted-by: OpenAI Codex: model version unavailable
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Summary
grp.getgrgid()conversion through__index__, including overflow versus missing-group errors, the-1sentinel, and bounded type names in diagnostics.longand unsignedlongconversion paths rather than collapsing out-of-range gids intoKeyError.Lib/test/test_grp.pyusingscripts/update_lib; the resulting file is byte-identical to pinned CPython and retires its sole expected-failure marker.CPython basis
Pinned source:
d220409a13b77de7a6c30abe16ea90db01528edc.grp_getgrgid_impl_Py_Gid_ConverterValidation
cargo checkand strict Clippy passed with a dedicated worktree target and fresh stdlib artifacts.__index__failures/non-integer results, and UTF-8-safe type-name truncation; none reaches an OS group database lookup.test_grpwas not run locally: it performs NSS/group-database queries. Actualtest_grpexecution on Unix CI is required before calling this integration validated. Windows lacks this Unix-only module; a skipped Windows module is not evidence of coverage.The underlying host lookup/error handling is otherwise unchanged. This does not claim validation on exotic signed-
gid_ttargets.Local checks used base
f98543dcdbe49fc902c17f28f81cc980fa69dd3f. Latest main observed before publication wasd24ca415d8ee8ac439e19ef50e624abb64c48482; intervening changes only touched JSON files, not this patch. Merge-result integration remains a CI responsibility, not a claimed local rebase test.AI assistance: OpenAI Codex; model version unavailable. The commit includes the corresponding
Assisted-bytrailer.Summary by CodeRabbit
TypeErrorand out-of-range IDs asOverflowError, rather than treating these cases as missing groups.KeyError, with the requested group ID included in the message.