GeezerVison

crates/derive-impl/src/pymodule.rs (1)

840-841: Cache the result of meta.module()? to avoid the double call.

meta.module()? is called twice in consecutive lines — once to check is_some() and once to get the value. A single call with caching is cleaner.

♻️ Proposed refactor

-        let has_module = meta.module()?.is_some();
-        let module_name = meta.module()?.unwrap_or_else(|| args.context.name.clone());
+        let module_opt = meta.module()?;
+        let has_module = module_opt.is_some();
+        let module_name = module_opt.unwrap_or_else(|| args.context.name.clone());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/derive-impl/src/pymodule.rs` around lines 840 - 841, Cache the result
of meta.module()? into a local variable instead of calling it twice: call
meta.module()? once (e.g., let module_opt = meta.module()?;) then set has_module
= module_opt.is_some(); and compute module_name = module_opt.unwrap_or_else(||
args.context.name.clone()); update uses of has_module and module_name
accordingly to avoid duplicate calls to meta.module()?.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/derive-impl/src/pymodule.rs (1)

233-243: Consider using .expect() with submodule name for better panic diagnostics.

All .unwrap() calls here are consistent with the existing patterns elsewhere in this file (e.g., lines 798, 892). However, since submodule initialization adds a layer of indirection, a panic during init could be hard to diagnose. Using .expect() with the submodule name would help.

💡 Proposed improvement for diagnostics

                 submodule_inits.push(quote! {
                     #(`#cfgs`)*
                     {
                         let child_def = `#mod_ident`::module_def(ctx);
-                        let child = child_def.create_module(vm).unwrap();
-                        child.__init_methods(vm).unwrap();
-                        `#mod_ident`::module_exec(vm, &child).unwrap();
+                        let child = child_def.create_module(vm)
+                            .expect(concat!("failed to create submodule ", `#py_name`));
+                        child.__init_methods(vm)
+                            .expect(concat!("failed to init methods for submodule ", `#py_name`));
+                        `#mod_ident`::module_exec(vm, &child)
+                            .expect(concat!("failed to exec submodule ", `#py_name`));
                         let child: ::rustpython_vm::PyObjectRef = child.into();
                         vm.__module_set_attr(module, ctx.intern_str(`#py_name`), child).unwrap();
                     }
                 });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/derive-impl/src/pymodule.rs` around lines 233 - 243, Replace the four
.unwrap() calls in the submodule initialization block pushed into
submodule_inits with .expect(...) so panics include the submodule name (use the
`#py_name` variable). Specifically update the calls to
`#mod_ident`::module_def(ctx).create_module(vm).unwrap(),
child.__init_methods(vm).unwrap(), `#mod_ident`::module_exec(vm, &child).unwrap(),
and vm.__module_set_attr(...).unwrap() to use .expect() messages that reference
`#py_name` (e.g., "failed to create/init/exec/set attr for submodule {`#py_name`}")
so failures during submodule init produce clear diagnostics for the named
submodule.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro