Actionable comments posted: 1

crates/capi/src/abstract_/mapping.rs (1)

10-38: 💤 Low value

Consider extracting shared logic into a helper to reduce duplication.

All three functions follow the same pattern: get mapping, call method, iterate to list. Per coding guidelines, when branches differ only in a method call but share common logic, extracting the common parts improves maintainability.

♻️ Possible helper extraction

+fn mapping_to_list(
+    obj: *mut PyObject,
+    get_items: impl FnOnce(&rustpython_vm::protocol::PyMapping, &rustpython_vm::VirtualMachine) -> rustpython_vm::PyResult<rustpython_vm::PyObjectRef>,
+) -> *mut PyObject {
+    with_vm(|vm| {
+        let obj = unsafe { &*obj };
+        let items = get_items(&obj.try_mapping(vm)?, vm)?;
+        let iter = items.get_iter(vm)?;
+        Ok(vm.ctx.new_list(iter.try_to_value(vm)?))
+    })
+}
+
 #[unsafe(no_mangle)]
 pub unsafe extern "C" fn PyMapping_Keys(obj: *mut PyObject) -> *mut PyObject {
-    with_vm(|vm| {
-        let obj = unsafe { &*obj };
-        let keys = obj.try_mapping(vm)?.keys(vm)?;
-        let iter = keys.get_iter(vm)?;
-        Ok(vm.ctx.new_list(iter.try_to_value(vm)?))
-    })
+    mapping_to_list(obj, |m, vm| m.keys(vm))
 }

Apply similarly to PyMapping_Values and PyMapping_Items.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/capi/src/abstract_/mapping.rs` around lines 10 - 38, Extract the
repeated logic in PyMapping_Keys, PyMapping_Values, and PyMapping_Items into a
small helper (e.g., fn map_to_list(obj: *mut PyObject, f: impl FnOnce(&dyn
Mapping, &VirtualMachine) -> PyResult<PyObjectRef>) -> *mut PyObject or a helper
taking an enum/closure) that: calls with_vm, dereferences obj, calls
obj.try_mapping(vm)?, invokes the specific mapping method (keys/values/items)
via the passed closure or selector, gets the iterator via get_iter(vm)?,
converts it with try_to_value(vm) and returns vm.ctx.new_list(...); then replace
the bodies of PyMapping_Keys, PyMapping_Values and PyMapping_Items to call this
helper with the appropriate mapping method selector so the only differing part
is the method passed in.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/capi/src/abstract_/mapping.rs`:
- Around line 40-66: Change the test module attribute from the disabled
#[cfg(false)] to #[cfg(test)] (or #[cfg(test)] plus #[ignore] if you want it
compiled but skipped), and update the test function size_keys_values_items to
call the RustPython C-API wrappers instead of only exercising PyO3: keep
Python::attach and creation of the PyDict/PyMapping, then invoke the C-API
functions (e.g., PyMapping_Size, PyMapping_Keys, PyMapping_Values,
PyMapping_Items or their crate wrapper functions) against the mapping and assert
their return values/lengths and that items are tuples, so the test actually
validates the C-API implementation rather than only PyO3 helpers.

---

Nitpick comments:
In `@crates/capi/src/abstract_/mapping.rs`:
- Around line 10-38: Extract the repeated logic in PyMapping_Keys,
PyMapping_Values, and PyMapping_Items into a small helper (e.g., fn
map_to_list(obj: *mut PyObject, f: impl FnOnce(&dyn Mapping, &VirtualMachine) ->
PyResult<PyObjectRef>) -> *mut PyObject or a helper taking an enum/closure)
that: calls with_vm, dereferences obj, calls obj.try_mapping(vm)?, invokes the
specific mapping method (keys/values/items) via the passed closure or selector,
gets the iterator via get_iter(vm)?, converts it with try_to_value(vm) and
returns vm.ctx.new_list(...); then replace the bodies of PyMapping_Keys,
PyMapping_Values and PyMapping_Items to call this helper with the appropriate
mapping method selector so the only differing part is the method passed in.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 1d45ea1b-0816-4cf2-9aa0-8956af0e0836

Reviewing files that changed from the base of the PR and between 1ce37bf and 5cb4197.

• crates/capi/src/abstract_.rs
• crates/capi/src/abstract_/mapping.rs

👍