Fix inheritance documentation with Sphinx RST syntax and method grouping #231
Conversation
Signed-off-by: Khushi Agrawal <[email protected]>
📝 WalkthroughWalkthroughClass inheritance metadata is tracked from parsing through resolution into code generation: base-class names are recorded when attaching inherited methods, propagated as Changes
Sequence Diagram(s)sequenceDiagram
%% Styling note: changed/added interactions are highlighted with green notes
autonumber
participant P as PXDParser
participant R as DeclResolver
participant C as CodeGenerator
participant FS as Files/Tests
Note over P,R: Parse .pxd and produce CppClassDecls
P->>R: attach_base_methods(transformed_methods, base_class_name)
Note right of R: R records base name per method\nand stores in class declaration
R->>C: pass resolved class decls (with inherited_method_bases)
Note left of C: CodeGenerator receives inherited context\nand threads `inherited_from` through APIs
C->>C: create wrappers & typestubs\n(include `inherited_from` annotations)
C->>FS: write updated runtime wrappers & .pyi stubs
FS->>User: tests assert docstring lengths and docs reflect\n:py:class: links for inherited classes
Note over P,C: (new) Sphinx demo script automates these steps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 <[email protected]>
![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 <[email protected]>
|
@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 <[email protected]>
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)
2 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.