Fix inheritance documentation with Sphinx RST syntax and method grouping #231
Conversation
Signed-off-by: Khushi Agrawal <149886195+khushiiagrawal@users.noreply.github.com>
📝 WalkthroughWalkthroughInheritance origins are recorded during PXD parsing and resolution, propagated as Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant P as PXDParser
participant R as DeclResolver
participant C as CodeGenerator
participant F as Files/Tests
participant U as User
Note over P: Parse .pxd → CppClassDecls\n(record base classes)
P->>R: attach_base_methods(transformed_methods,\n base_class_name)
Note right of R: Record `inherited_method_bases` per method
R->>C: provide resolved class decls (include inherited_method_bases)
Note left of C: Thread `inherited_from` into wrapper & stub creation
C->>F: write runtime wrappers & `.pyi` stubs (annotated)
F->>U: tests/docs verify :py:class: links and inherited annotations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
@jpfeuffer Please take a look. |
|
Looks good but to be sure, we would need to see a sphinx documentation page with autodoc for this. |
Signed-off-by: Khushi Agrawal <149886195+khushiiagrawal@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
create_sphinx_demo.py (4)
50-53: Consider using unpacking for more idiomatic list construction.The list concatenation works correctly, but using the unpacking operator
*is more idiomatic in modern Python.🔎 Proposed refactor
run_command([ "autowrap", - "--out", "moduleB.pyx" - ] + pxd_files) + "--out", "moduleB.pyx", + *pxd_files + ])
122-139: Simplify the conf.py modification logic.The logic to insert
sys.path.inserthas several issues:
- Redundant check at line 130 (already checked at line 122)
- The variable
new_linesmay not be defined if the condition at line 129 never matches- The
isinstance(new_lines, list)check at line 139 is unnecessary sincenew_linesis always a list when defined🔎 Proposed refactor for clearer logic
# Add path configuration if not already present if "sys.path.insert" not in conf_content: - # Find the extensions line and add after it - lines = conf_content.split("\n") - new_lines = [] - for i, line in enumerate(lines): - new_lines.append(line) - # Add after extensions or after imports - if "extensions = [" in line or (i > 0 and "import sys" in lines[i-1] and "import os" in line): - if "sys.path.insert" not in conf_content: - new_lines.append("") - new_lines.append("# Add current directory to path for autodoc") - new_lines.append("import sys") - new_lines.append("import os") - new_lines.append("sys.path.insert(0, os.path.abspath('.'))") - break - - with open(conf_py_path, "w") as f: - f.write("\n".join(new_lines) if isinstance(new_lines, list) else conf_content) + # Prepend the path configuration at the top + path_setup = """import sys +import os + +# Add current directory to path for autodoc +sys.path.insert(0, os.path.abspath('.')) + +""" + conf_content = path_setup + conf_content + + with open(conf_py_path, "w") as f: + f.write(conf_content)
226-231: Remove unnecessary f-string prefixes.Lines 226 and 228 use f-strings without any placeholders. Regular strings would be clearer.
🔎 Proposed refactor
- print(f"\nHTML documentation is available at:") + print("\nHTML documentation is available at:") print(f" {html_path}") - print(f"\nTo view it, run:") + print("\nTo view it, run:") print(f" open {html_path} # macOS")
234-240: Consider catching more specific exceptions.Catching bare
Exceptionis overly broad. For better error handling, catch specific exceptions thatwebbrowser.open()might raise, such asOSErrororwebbrowser.Error.🔎 Proposed refactor
# Try to open automatically try: import webbrowser webbrowser.open(f"file://{html_path}") print("\nOpened in default browser!") - except Exception as e: + except (OSError, webbrowser.Error) as e: print(f"\nCould not open browser automatically: {e}") print("Please open the file manually.")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
create_sphinx_demo.py
🧰 Additional context used
🧬 Code graph analysis (1)
create_sphinx_demo.py (1)
tests/test_files/templated.hpp (1)
f(61-64)
🪛 Ruff (0.14.10)
create_sphinx_demo.py
13-13: subprocess call: check for execution of untrusted input
(S603)
50-53: Consider ["autowrap", "--out", "moduleB.pyx", *pxd_files] instead of concatenation
Replace with ["autowrap", "--out", "moduleB.pyx", *pxd_files]
(RUF005)
104-104: Local variable quickstart_input is assigned to but never used
Remove assignment to unused variable quickstart_input
(F841)
226-226: f-string without any placeholders
Remove extraneous f prefix
(F541)
228-228: f-string without any placeholders
Remove extraneous f prefix
(F541)
238-238: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
create_sphinx_demo.py (1)
10-25: No security concern—allrun_commandcalls already use list-based commands.All four call sites of
run_command(lines 50, 88, 105, and 219) pass list-based commands, which meansshell=Falseis always used. The hard-coded commands reference only known tools (autowrap,sphinx-quickstart,sphinx-build,python) and project paths, with no user input interpolation. This script follows the recommended best practice and has no security risk.
@jpfeuffer I've generated Sphinx documentation demonstrating the inheritance fixes. Here's what the screenshot shows: 
Inheritance link in class docstring: "— Inherits from A_second" - The HTML file is attached, and you can regenerate it using |
|
Looks pretty good. Can you demonstrate the method grouping. I only see one method. |
Signed-off-by: Khushi Agrawal <149886195+khushiiagrawal@users.noreply.github.com>
|
@jpfeuffer Thank you for the feedback! I've investigated and fixed the issues: 1. Method GroupingI found and fixed a bug in The
2. Broken LinkI checked all links in the generated HTML. All 3. Inherited Methods OrderCurrently, inherited methods appear after class-defined methods (following Python documentation conventions). I'm happy to change this to put inherited methods first if you prefer. What would you like? 
Screen.Recording.2026-01-02.at.11.07.10.AM.mov |
Signed-off-by: Khushi Agrawal <149886195+khushiiagrawal@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
create_sphinx_demo.py (2)
97-110: Remove unused sphinx_config and quickstart_input variables.The past review comment correctly identifies this issue: these variables are defined but never used since
sphinx-quickstartis invoked with non-interactive command-line flags.
29-29: Incorrect project root calculation will break the script.The past review comment correctly identifies this issue:
Path(__file__).parentresolves tosrc/scripts/, not the project root. This will cause failures when accessing test files at lines 46-48.
🧹 Nitpick comments (3)
create_sphinx_demo.py (3)
7-7: Remove unused import.The
tempfilemodule is imported but never used in the script.🔎 Proposed fix
import os import sys import shutil import subprocess -import tempfile from pathlib import Path
51-54: Consider using unpacking for cleaner list construction.The list concatenation can be simplified using the unpacking operator.
🔎 Proposed refactor
- run_command([ - "autowrap", - "--out", "moduleB.pyx" - ] + pxd_files) + run_command([ + "autowrap", + "--out", "moduleB.pyx", + *pxd_files + ])
228-233: Remove unnecessary f-string prefixes.Lines 228 and 230 use f-string prefixes but contain no placeholders.
🔎 Proposed refactor
- print(f"\nHTML documentation is available at:") + print("\nHTML documentation is available at:") print(f" {html_path}") - print(f"\nTo view it, run:") + print("\nTo view it, run:") print(f" open {html_path} # macOS") print(f" xdg-open {html_path} # Linux") print(f" start {html_path} # Windows")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
create_sphinx_demo.py
🧰 Additional context used
🪛 Ruff (0.14.10)
create_sphinx_demo.py
13-13: subprocess call: check for execution of untrusted input
(S603)
51-54: Consider ["autowrap", "--out", "moduleB.pyx", *pxd_files] instead of concatenation
Replace with ["autowrap", "--out", "moduleB.pyx", *pxd_files]
(RUF005)
106-106: Local variable quickstart_input is assigned to but never used
Remove assignment to unused variable quickstart_input
(F841)
228-228: f-string without any placeholders
Remove extraneous f prefix
(F541)
230-230: f-string without any placeholders
Remove extraneous f prefix
(F541)
240-240: Do not catch blind exception: Exception
(BLE001)
|
@jpfeuffer PTAL. |
|
Looks good. The methods are not really sorted by inherited or not though. Might be a sphinx configuration issue. |
Signed-off-by: Khushi Agrawal <149886195+khushiiagrawal@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
create_sphinx_demo.py (3)
29-29: Incorrect project root calculation will break the script.The script is located at
src/scripts/create_sphinx_demo.py, soPath(__file__).parentonly resolves tosrc/scripts/, not the project root. This will cause failures when accessing test files at lines 46-47.
97-110: Remove unused sphinx_config and quickstart_input variables.The
sphinx_configlist andquickstart_inputstring are defined but never used, since thesphinx-quickstartcommand is invoked with non-interactive command-line flags (-q,--sep, etc.).
118-144: NameError when sys.path.insert already present in conf.py.If
conf_py_pathexists but already contains"sys.path.insert"(line 124 check fails), the variablenew_linesis never defined. However, line 144 unconditionally referencesnew_lines, causing aNameError. This will crash the script when run multiple times or if conf.py already has the path configuration.
🧹 Nitpick comments (2)
create_sphinx_demo.py (2)
51-54: Consider using unpacking operator for cleaner list construction.The list concatenation works correctly but using the unpacking operator
*pxd_fileswould be more idiomatic Python.🔎 Proposed refactor
- run_command([ - "autowrap", - "--out", "moduleB.pyx" - ] + pxd_files) + run_command([ + "autowrap", + "--out", "moduleB.pyx", + *pxd_files + ])
234-240: Remove unnecessary f-string prefixes.Lines 234 and 236 use f-strings without any placeholders. These can be regular strings.
🔎 Proposed refactor
- print(f"\nHTML documentation is available at:") + print("\nHTML documentation is available at:") print(f" {html_path}") - print(f"\nTo view it, run:") + print("\nTo view it, run:") print(f" open {html_path} # macOS") print(f" xdg-open {html_path} # Linux") print(f" start {html_path} # Windows")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
create_sphinx_demo.py
🧰 Additional context used
🪛 Ruff (0.14.10)
create_sphinx_demo.py
13-13: subprocess call: check for execution of untrusted input
(S603)
51-54: Consider ["autowrap", "--out", "moduleB.pyx", *pxd_files] instead of concatenation
Replace with ["autowrap", "--out", "moduleB.pyx", *pxd_files]
(RUF005)
106-106: Local variable quickstart_input is assigned to but never used
Remove assignment to unused variable quickstart_input
(F841)
234-234: f-string without any placeholders
Remove extraneous f prefix
(F541)
236-236: f-string without any placeholders
Remove extraneous f prefix
(F541)
246-246: Do not catch blind exception: Exception
(BLE001)
I verified the fix using a Sphinx demo. With autodoc_member_order = 'bysource' in conf.py, the documentation correctly respects the order of methods in the Cython module: Class-defined methods (callB) appear first. 
|
Signed-off-by: Khushi Agrawal <149886195+khushiiagrawal@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_code_generator_libcpp.py (1)
114-115: Consider verifying Sphinx syntax presence in addition to length.The updated test checks docstring length but doesn't verify that the Sphinx RST syntax is actually present. Testing for content rather than (or in addition to) length would make this test more robust and directly validate the PR's feature.
🔎 Suggested enhancement to verify Sphinx syntax
# Length changed due to Sphinx RST syntax (:py:class:`AbstractBaseClass` instead of AbstractBaseClass) assert len(libcpp.ABS_Impl1.__doc__) == 98 +assert ":py:class:`AbstractBaseClass`" in libcpp.ABS_Impl1.__doc__This addition would verify that the Sphinx cross-reference syntax is actually generated, making the test less brittle to unrelated formatting changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_code_generator_libcpp.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_code_generator_libcpp.py (1)
tests/test_files/libcpp_test.hpp (2)
ABS_Impl1(31-31)ABS_Impl1(32-32)
|
@jpfeuffer PTAL |
|
@jpfeuffer good to go? |
|
Yeah I guess we can try to see how it looks with pyopenms. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
autowrap/CodeGenerator.py (2)
571-580: Consider more robust type handling forwrap-inheritsannotation.The code assumes
inherit_annotis either a list or a single string (line 574), but doesn't validate this assumption. If the annotation is accidentally set to another type (dict, tuple, int, etc.), the code may fail unexpectedly.Additionally, the template extraction at line 578 (
base_class.split('[')[0].strip()) is simplistic and could fail for nested templates like"Base<Wrapper<T>>". While this may work for the current use case, it's worth noting.🛡️ Proposed defensive type validation
inherit_annot = r_class.cpp_decl.annotations.get("wrap-inherits", []) if inherit_annot: + # Ensure inherit_annot is a list + if isinstance(inherit_annot, str): + inherit_list = [inherit_annot] + elif isinstance(inherit_annot, list): + inherit_list = inherit_annot + else: + raise ValueError(f"wrap-inherits annotation must be a string or list, got {type(inherit_annot)}") - # Generate Sphinx RST links for inherited classes - inherit_list = inherit_annot if isinstance(inherit_annot, list) else [inherit_annot] inherit_links = [] for base_class in inherit_list: + if not base_class or not isinstance(base_class, str): + continue # Skip empty or invalid entries # Extract class name (handle template syntax like "Base[A]") base_name = base_class.split('[')[0].strip() inherit_links.append(":py:class:`%s`" % base_name) - docstring += " -- Inherits from %s\n" % ", ".join(inherit_links) + if inherit_links: + docstring += " -- Inherits from %s\n" % ", ".join(inherit_links)
1850-1879: Consider minor refactoring to reduce code duplication.The method signature generation (lines 1854-1860 and 1873-1879) is duplicated in both branches. While the logic is correct and the duplication is minimal, consolidating the signature generation could improve maintainability.
♻️ Proposed consolidation to reduce duplication
+ # Generate method signature (same for integral and non-integral) + meth_code.add( + """ + |def __setitem__(self, $in_t_cy key, $res_t_base value): + | \"\"\"$docstring\"\"\" + """, + locals(), + ) + # Check if the key type is integral for bounds checking is_integral = self._is_integral_type(ctype_in) size_guard = mdcl.cpp_decl.annotations.get("wrap-upper-limit") - if is_integral: - meth_code.add( - """ - |def __setitem__(self, $in_t_cy key, $res_t_base value): - | \"\"\"$docstring\"\"\" - """, - locals(), - ) - if size_guard: - meth_code.add( - """ - | cdef int _idx = $call_arg - | if _idx < 0: - | raise IndexError("invalid index %d" % _idx) - | if _idx >= self.inst.get().$size_guard: - | raise IndexError("invalid index %d" % _idx) - """, - locals(), - ) - else: + # Apply bounds checking only for integral types with size guard + if is_integral and size_guard: meth_code.add( """ - |def __setitem__(self, $in_t_cy key, $res_t_base value): - | \"\"\"$docstring\"\"\" + | cdef int _idx = $call_arg + | if _idx < 0: + | raise IndexError("invalid index %d" % _idx) + | if _idx >= self.inst.get().$size_guard: + | raise IndexError("invalid index %d" % _idx) """, locals(), )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
autowrap/CodeGenerator.py
🔇 Additional comments (5)
autowrap/CodeGenerator.py (5)
800-835: LGTM! Method grouping implementation is correct.The implementation successfully:
- Separates class-defined methods from inherited methods using
inherited_method_bases- Generates constructors first (line 807 comment confirms this is intentional)
- Generates class-defined methods alphabetically (line 819)
- Generates inherited methods alphabetically with
inherited_frommetadata (lines 827-834)- Safely retrieves
inherited_method_baseswith a default empty dict (line 801)The grouping strategy aligns with the PR objectives to group inherited methods separately after class-defined methods.
917-917: LGTM! Parameter threading for inheritance context is complete.The
inherited_fromparameter is correctly threaded through all relevant method signatures:
_create_overloaded_method_decl(line 917)create_wrapper_for_method(line 1041)_create_fun_decl_and_input_conversion(line 1165)create_wrapper_for_nonoverloaded_method(line 1397)The parameter propagates through both overloaded and non-overloaded code paths and correctly appears in the generated docstrings.
Also applies to: 1041-1041, 1165-1165, 1397-1397
991-992: LGTM! Inherited-from annotations are correctly added to docstrings.Both locations consistently:
- Check if
inherited_fromis truthy before adding the annotation- Use the Sphinx RST
:py:class:syntax for cross-references- Apply proper indentation (8 spaces) for docstring formatting
This aligns with the PR objective to annotate inherited methods with their origin class.
Also applies to: 1219-1222
1753-1766: LGTM! Bounds checking correctly gated by integral type detection.The modification improves type safety by:
- Applying integer-specific bounds checking only to integral index types (line 1753)
- Avoiding inappropriate bounds checks on non-integral keys (e.g., string keys in maps)
The logic correctly requires both
is_integralandsize_guardto be truthy before applying bounds checks (line 1756).
1728-1734: No changes needed—the implementation correctly handles unsigned integral types.The
_is_integral_typemethod properly detects both signed and unsigned variants becauseCppTypestores type information as separate attributes:base_type(e.g.,"int","long") andis_unsigned(a boolean flag). When parsing types like"unsigned int", the parser strips theunsignedprefix, storesis_unsigned=True, and keeps only"int"inbase_type. Therefore, bothunsigned intandinthave the samebase_typevalue and are correctly identified as integral types. No expansion of theintegral_base_typesset to include explicit"unsigned int"entries is necessary.Likely an incorrect or invalid review comment.
|
Does the rabbit make sense? |
|
Yes! |
- Remove unused tempfile import (replaced with webbrowser at top level) - Remove unused sphinx_config and quickstart_input variables - Fix NameError when sys.path.insert already present in conf.py by simplifying the logic to prepend path configuration - Use list unpacking (*pxd_files) instead of concatenation - Remove unnecessary f-string prefixes on lines without placeholders - Use specific exception types (OSError, webbrowser.Error) instead of bare Exception Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1. Add defensive type validation for wrap-inherits annotation: - Validate that inherit_annot is a string or list, raise ValueError otherwise - Skip empty or non-string entries in the inherit list - Only add "Inherits from" if there are valid links 2. Reduce code duplication in __setitem__ generation: - Move method signature generation outside the if/else branches - Consolidate condition to `if is_integral and size_guard:` Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The script served its purpose of verifying that Sphinx RST syntax works correctly for inheritance documentation. The verification was demonstrated via screenshots in PR OpenMS#231. Future testing can be done with pyOpenMS directly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
autowrap/CodeGenerator.py (2)
579-581: Minor: Consider extracting error message to a constant.Ruff TRY003 suggests avoiding long messages directly in exception calls. While not critical, extracting the message to a module-level constant improves maintainability.
♻️ Optional refactor
At module level:
_WRAP_INHERITS_TYPE_ERROR = "wrap-inherits annotation must be a string or list, got {}"Then:
- raise ValueError( - f"wrap-inherits annotation must be a string or list, got {type(inherit_annot).__name__}" - ) + raise ValueError(_WRAP_INHERITS_TYPE_ERROR.format(type(inherit_annot).__name__))
584-586: Consider logging skipped invalid entries.Invalid or empty base class entries in wrap-inherits are silently skipped. Adding a debug or warning log would help developers identify configuration issues.
💡 Suggested improvement
for base_class in inherit_list: if not isinstance(base_class, str) or not base_class: + if base_class: + L.warning(f"Skipping invalid wrap-inherits entry: {base_class!r}") continue # Skip empty or invalid entries
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
autowrap/CodeGenerator.pycreate_sphinx_demo.py
🧰 Additional context used
🧬 Code graph analysis (1)
autowrap/CodeGenerator.py (1)
autowrap/Code.py (1)
add(56-75)
🪛 Ruff (0.14.10)
create_sphinx_demo.py
13-13: subprocess call: check for execution of untrusted input
(S603)
autowrap/CodeGenerator.py
579-581: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (9)
create_sphinx_demo.py (3)
13-13: Static analysis false positive - no action needed.The Ruff S603 warning is a false positive. This script uses
subprocess.runwith controlled, trusted command arguments constructed from hardcoded paths and values. There is no untrusted user input being executed.
97-101: Verifycheck=Falseis intentional for sphinx-quickstart.Setting
check=Falsesuppresses exceptions fromsphinx-quickstart, which could hide legitimate errors (e.g., missing Sphinx installation, filesystem issues). If the command can fail non-critically (e.g., when a Sphinx project already exists), consider adding explicit error handling or logging to inform the user.💡 Suggested improvement
- run_command( - ["sphinx-quickstart", "-q", "--sep", "-p", "Autowrap Demo", - "-a", "Test", "-v", "1.0", "--ext-autodoc", "--ext-viewcode", "."], - check=False - ) + result = run_command( + ["sphinx-quickstart", "-q", "--sep", "-p", "Autowrap Demo", + "-a", "Test", "-v", "1.0", "--ext-autodoc", "--ext-viewcode", "."], + check=False + ) + if result.returncode != 0: + print("Warning: sphinx-quickstart returned non-zero exit code (may be expected if project exists)")
10-246: Well-structured demo script with clear step delineation.The script provides an excellent automated demonstration of the inheritance documentation improvements. The 7-step process is well-documented, and the error handling for browser opening (lines 227-232) is robust.
autowrap/CodeGenerator.py (6)
571-591: Robust validation with Sphinx RST link generation.The wrap-inherits annotation validation correctly handles both string and list inputs, and the Sphinx
:py:class:link generation properly handles template syntax (line 588). The validation raises appropriate errors for invalid types.
811-846: Excellent method grouping with proper inheritance context.The separation of class-defined and inherited methods is well-implemented:
- Constructor handling is preserved (lines 818-823)
- Class-defined methods are generated first, then inherited methods (lines 829-845)
- Both groups are sorted alphabetically for consistency
- The
inherited_fromparameter is correctly propagated to annotate inherited methodsThis implementation aligns perfectly with the PR objectives to group inherited methods separately with clear origin annotations.
928-928: Complete and consistentinherited_frompropagation.The
inherited_fromparameter is properly threaded through the entire wrapper generation pipeline:
- Added to all relevant method signatures with
Nonedefault (backward compatible)- Correctly propagated through overloaded and non-overloaded method paths
- Annotations use Sphinx
:py:class:syntax for cross-references in both runtime docstrings and type stubs- The implementation ensures inherited methods are documented with their origin class
Also applies to: 1002-1003, 1052-1052, 1151-1151, 1171-1171, 1176-1176, 1230-1233, 1408-1408, 1417-1417
1747-1812: Improved getitem with proper integral type bounds checking.The refactored
create_special_getitem_methodcorrectly gates integer-specific bounds checking (lines 1767-1777) using the new_is_integral_typehelper. This prevents incorrect bounds checks on non-integer index types (e.g., string keys for maps).
1815-1898: Excellent setitem refactoring reduces code duplication.The consolidated signature generation (lines 1864-1871) and conditional bounds checking (lines 1873-1884) eliminate duplication from the previous implementation. The logic correctly:
- Generates the method signature once for all key types
- Applies bounds checking only for integral types with size guards
- Maintains consistency with the getitem implementation
Based on PR comments, this addresses the CodeRabbit review feedback to reduce duplicated code in
__setitem__generation.
1739-1745: The integral type set is already complete — no changes needed.The
_is_integral_typecheck correctly covers all necessary integral types. When types likeunsigned longorunsigned intare parsed, the base components (long,int) are extracted and stored separately from theis_unsignedflag (as separate attributes inCppType). The method checks onlycpp_type.base_type, which already includes all necessary base types:int,long,short,char, plus all fixed-width variants (int8_tthroughuint64_t).Test files show all operator[] declarations use
size_t(already in the set), and there are no uses of wide character types (wchar_t,char16_t,char32_t) in the codebase.Likely an incorrect or invalid review comment.
|
Thanks! |
Changes in this release: - Fix inheritance documentation with Sphinx RST syntax and method grouping (#231) - Fix circular import issue with cross-module scoped enum type checking (#238) - Fix cross-module enum class imports for type assertions (#237) - Add test for enum-based overload resolution (#234) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3 participants
Summary
Fixes two issues with inheritance documentation:
:py:class:BaseClass`` instead of plain text for inheritance linksBaseClass." notationFixes #180
Changes
CodeGenerator.py: Generate Sphinx RST syntax in class docstrings; group inherited methods after class-defined methodsPXDParser.py: Track inherited methods with base class namesDeclResolver.py: Pass base class name when attaching inherited methodstest_full_library.py: Update expected docstring length (93→101 chars)Testing
.pyifile docstringsTest case:
Bklassinheriting fromA_second- inheritscallA2()methodSummary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.