Fix handling of wrap-ignore for base classes to avoid invalid cimport… #196
Conversation
… in derived classes Previously, autowrap would generate a `from ... cimport` statement for derived classes even if their base class was marked with `wrap-ignore`. This caused build failures due to missing .pxd files for ignored base classes, especially when those were the only contents of their file. This change checks whether the base class has the `wrap-ignore` annotation and, if so, suppresses the cimport generation for the derived class. This prevents erroneous imports like: From ._pyopenms_20 cimport SpectrumAccessTransforming When SpectrumAccessTransforming is intentionally not wrapped. Also adds an optional debug message to clarify when this skip occurs. This fix preserves expected behaviour for abstract base classes and improves compatibility when selectively wrapping a class hierarchy. Fixes: OpenMS#194
WalkthroughThe update modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant CodeGenerator
participant ResolvedClass
participant BaseClass
CodeGenerator->>ResolvedClass: Iterate over resolved classes
ResolvedClass->>BaseClass: Check base class for wrap-ignore
alt BaseClass is wrap-ignored or no_pxd_import
CodeGenerator-->>CodeGenerator: Skip cimport statement
else
CodeGenerator-->>CodeGenerator: Add cimport statement
end
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
autowrap/CodeGenerator.py (1)
1954-1962: LGTM! Well-implemented fix for wrap-ignore inheritance issue.The added logic correctly addresses the issue where derived classes would generate invalid cimport statements when their base classes are marked with
wrap-ignore. The implementation is clean and follows existing code patterns.However, consider using the logging framework for consistency:
- print(f"[autowrap] Skipping pxd import for derived class '{resolved.name}' " - f"because base '{base_class.name}' is wrap-ignored.") + L.info(f"Skipping pxd import for derived class '{resolved.name}' " + f"because base '{base_class.name}' is wrap-ignored.")This would be more consistent with other logging statements in the file (e.g., lines 1927, 1969).
|
Nice but such a breaking change urgently needs tests. |
|
Also, not even the old tests pass.. |
… in derived classes
Previously, autowrap would generate a
from ... cimportstatement for derived classes even if their base class was marked withwrap-ignore. This caused build failures due to missing .pxd files for ignored base classes, especially when those were the only contents of their file.This change checks whether the base class has the
wrap-ignoreannotation and, if so, suppresses the cimport generation for the derived class. This prevents erroneous imports like:From ._pyopenms_20 cimport SpectrumAccessTransforming
When SpectrumAccessTransforming is intentionally not wrapped.
Also adds an optional debug message to clarify when this skip occurs.
This fix preserves expected behaviour for abstract base classes and improves compatibility when selectively wrapping a class hierarchy.
Fixes: #194
Summary by CodeRabbit