Walkthrough

Refactors winreg error handling: os_error_from_windows_code now takes only (vm, code); SetValueEx's value_name changed to Option<String>; call sites updated to use unified OS error mapping and removed per-call formatted error messages.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Rewrite of winreg module and bump to 3.13.2 #5594: Modifies the same winreg.rs functions—centralizing os_error_from_windows_code and updating SetValueEx/value-name handling.

Poem

🐰 I hopped through code with nimble paws,
Streamlined errors, trimmed the claws,
One code, one call, no stringy fray,
Registry dances tidy today!

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

• 📝 Generate docstrings

• Create PR with unit tests
• Post copyable unit tests in a comment

• Auto-commit resolved conflicts to branch winreg
• Post resolved changes as copyable diffs in a comment

No actionable comments were generated in the recent review. 🎉

crates/vm/src/stdlib/winreg.rs (2)

297-298: Remaining call sites still use the old vm.new_os_error(format!("error code: ...")) pattern.

Since this PR's goal is to fix winreg error handling, consider migrating the remaining ~15 call sites (e.g., ConnectRegistry, CreateKey, CreateKeyEx, DeleteKey, DeleteValue, DeleteKeyEx, FlushKey, LoadKey, QueryInfoKey, SaveKey, DisableReflectionKey, EnableReflectionKey, QueryReflectionKey, EnumValue, PyHkey::Close) to use os_error_from_windows_code(vm, res as i32) for consistent, human-readable Windows error descriptions.

Example fix for ConnectRegistry (same pattern applies elsewhere)

         if res == 0 {
             Ok(PyHkey::new(ret_key))
         } else {
-            Err(vm.new_os_error(format!("error code: {res}")))
+            Err(os_error_from_windows_code(vm, res as i32))
         }

---

278-310: Duplicated logic across Some/None branches.

The two branches differ only in the pointer value passed for computer_name but duplicate the entire FFI call and error handling.

Suggested consolidation

 fn ConnectRegistry(
     computer_name: Option<String>,
     key: PyRef<PyHkey>,
     vm: &VirtualMachine,
 ) -> PyResult<PyHkey> {
-    if let Some(computer_name) = computer_name {
-        let mut ret_key = core::ptr::null_mut();
-        let wide_computer_name = computer_name.to_wide_with_nul();
-        let res = unsafe {
-            Registry::RegConnectRegistryW(
-                wide_computer_name.as_ptr(),
-                key.hkey.load(),
-                &mut ret_key,
-            )
-        };
-        if res == 0 {
-            Ok(PyHkey::new(ret_key))
-        } else {
-            Err(vm.new_os_error(format!("error code: {res}")))
-        }
-    } else {
-        let mut ret_key = core::ptr::null_mut();
-        let res = unsafe {
-            Registry::RegConnectRegistryW(core::ptr::null_mut(), key.hkey.load(), &mut ret_key)
-        };
-        if res == 0 {
-            Ok(PyHkey::new(ret_key))
-        } else {
-            Err(vm.new_os_error(format!("error code: {res}")))
-        }
-    }
+    let wide_name = computer_name.as_deref().map(|s| s.to_wide_with_nul());
+    let name_ptr = wide_name
+        .as_deref()
+        .map_or(core::ptr::null(), |s| s.as_ptr());
+    let mut ret_key = core::ptr::null_mut();
+    let res = unsafe {
+        Registry::RegConnectRegistryW(name_ptr, key.hkey.load(), &mut ret_key)
+    };
+    if res == 0 {
+        Ok(PyHkey::new(ret_key))
+    } else {
+        Err(os_error_from_windows_code(vm, res as i32))
+    }
 }

As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."

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.

• X
• Mastodon
• Reddit
• LinkedIn

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

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] test: cpython/Lib/test/test_launcher.py

dependencies:

dependent tests: (no tests depend on launcher)

[x] lib: cpython/Lib/urllib
[x] test: cpython/Lib/test/test_urllib.py
[x] test: cpython/Lib/test/test_urllib2.py
[x] test: cpython/Lib/test/test_urllib2_localnet.py (TODO: 19)
[x] test: cpython/Lib/test/test_urllib2net.py
[x] test: cpython/Lib/test/test_urllibnet.py
[x] test: cpython/Lib/test/test_urlparse.py
[x] test: cpython/Lib/test/test_urllib_response.py
[x] test: cpython/Lib/test/test_robotparser.py

dependencies:

• urllib

dependent tests: (27 tests)

• urllib: test_genericalias test_http_cookiejar test_httpservers test_logging test_pathlib test_pydoc test_robotparser test_site test_sqlite3 test_ssl test_ucn test_urllib test_urllib2 test_urllib2_localnet test_urllib2net test_urllib_response test_urllibnet test_urlparse
  • email.utils: test_email test_smtplib
    • smtplib: test_smtpnet
  • http.client: test_docxmlrpc test_hashlib test_unicodedata test_wsgiref test_xmlrpc
  • pydoc: test_enum

[x] test: cpython/Lib/test/test_winreg.py

dependencies:

dependent tests: (3 tests)

• winreg: test_importlib test_launcher test_winreg

Legend:

• [+] path exists in CPython
• [x] up-to-date, [ ] outdated