Conversation
… that are not parameterized
update across all parameter.rst pages
WalkthroughAdds a Sphinx directive that renders parameter tables from Python docstrings, updates several Parameters docs to use it, registers the extension in Sphinx, adds docstring-parser to docs deps and pyproject extras, and modifies pyopenms_viz._config dataclass behaviors (legend defaults, defensive copies, stricter update errors, marginals handling). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant S as Sphinx
participant D as docstring_to_table (directive)
participant M as pyopenms_viz._config (module)
participant P as docstring-parser
participant T as Sphinx AST table
Note over S,D: During documentation build
S->>D: encounter .. docstring_to_table:: with :docstring:
D->>M: import target object path
alt import success
M-->>D: object + docstring(s)
D->>P: parse docstring(s) (include parents if requested)
P-->>D: parameter metadata
D->>T: render 4-column table (Parameter, Type, Description, Default)
T-->>S: table inserted into AST
else import failure
D-->>S: emit error node with message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
✨ Finishing Touches🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
docs/_ext/docstring_to_table.py (3)
21-26: Annotate directive class attributes as ClassVar and use docutils converters.Prevents Ruff RUF012 and aligns with docutils expectations for option conversion.
-from docutils.parsers.rst import Directive, directives +from docutils.parsers.rst import Directive, directives +from typing import ClassVar, Any @@ -class DocstringToTableDirective(Directive): - has_content = False - required_arguments = 0 - optional_arguments = 0 - option_spec = { - "docstring": str, - "title": str, - "parent_depth": int, # Number of parent classes to include - "default_docstring": directives.flag, # Flag, no argument required - } +class DocstringToTableDirective(Directive): + has_content: ClassVar[bool] = False + required_arguments: ClassVar[int] = 0 + optional_arguments: ClassVar[int] = 0 + option_spec: ClassVar[dict[str, Any]] = { + "docstring": directives.unchanged_required, + "title": directives.unchanged, + "parent_depth": directives.nonnegative_int, # Number of parent classes to include + "default_docstring": directives.flag, # Flag, no argument required + }
68-80: Traverse full MRO instead of only the first base.Includes multiple inheritance and simplifies depth handling.
- # Collect docstrings from parent classes up to parent_depth - docstrings = [] - current_obj = obj - for i in range(parent_depth + 1): - docstring = inspect.getdoc(current_obj) - if docstring: - docstrings.append(docstring) - bases = getattr(current_obj, "__bases__", ()) - if bases and i < parent_depth: - current_obj = bases[0] - else: - break + # Collect docstrings from class MRO up to parent_depth (or the object itself if not a class) + docstrings = [] + if inspect.isclass(obj): + for base in inspect.getmro(obj)[: parent_depth + 1]: + ds = inspect.getdoc(base) + if ds: + docstrings.append(ds) + else: + ds = inspect.getdoc(obj) + if ds: + docstrings.append(ds)
120-145: Minor: prefer caption node for tables.nodes.caption is more idiomatic for table captions in Docutils.
- if table_title: - title_node = nodes.title(text=table_title) - table += title_node + if table_title: + table += nodes.caption(text=table_title)pyopenms_viz/_config.py (3)
137-141: Docstring accuracy: kind/z “Required” vs defaults; allowed kinds list.Fields default to None; “Required.” is misleading. Also allowed kinds include line, vline, scatter, complex in the type but not in the docstring.
- kind (Literal["chromatogram", "mobilogram", "spectrum", "peakmap"]): Type of plot. Required. - x (str): The column name for the x-axis data. Required. - y (str): The column name for the y-axis data. Required. - z (str): The column name for the z-axis data. Required. + kind (Literal["line", "vline", "scatter", "chromatogram", "mobilogram", "spectrum", "peakmap", "complex"]): Type of plot. Defaults to None. + x (str): The column name for the x-axis data. Defaults to None. + y (str): The column name for the y-axis data. Defaults to None. + z (str): The column name for the z-axis data (only for 3D/when applicable). Defaults to None.If you want to keep x/y as required in rendered docs, we can rely on DEFAULT_DOCSTRING to mark them with “*”.
176-178: Mark helper as static for clarity.Avoids confusion about implicit self.
- def default_legend_factory(): + @staticmethod + def default_legend_factory(): return LegendConfig(title="Trace")Apply the same to ChromatogramConfig.default_legend_factory (Line 313).
439-445: Validate marginal kind early.Raise a clear error for unsupported kinds to avoid None attribute errors later.
@staticmethod def marginal_config_factory(kind): if kind == "chromatogram": return ChromatogramConfig() if kind == "spectrum": return SpectrumConfig() + raise ValueError(f"Unsupported marginal kind: {kind!r}")docs/conf.py (1)
92-92: Avoid adding docs/ to sys.path; importing docs. already works via parent path.*The parent insertion at Line 25 is sufficient and less error-prone.
-sys.path.insert(0, os.path.abspath(".")) # Add docs/ to pathdocs/Parameters/Parameters.rst (1)
8-14: Include parent options where applicable.If BasePlotConfig/LegendConfig inherit fields, include parents; otherwise ignore. Titles look good.
.. docstring_to_table:: :docstring: pyopenms_viz._config.BasePlotConfig :title: Core Options + :parent_depth: 1 .. docstring_to_table:: :docstring: pyopenms_viz._config.LegendConfig :title: Legend Options + :parent_depth: 1docs/Parameters/Spectrum.rst (1)
10-13: Add a title and drop empty option.Align with other pages by adding a table title and removing the empty
:default_docstring:... docstring_to_table:: :docstring: pyopenms_viz._config.SpectrumConfig :parent_depth: 1 - :default_docstring: + :title: Spectrum Optionsdocs/Parameters/Chromatogram.rst (1)
9-12: Add a title and remove empty option.Match the pattern used elsewhere.
.. docstring_to_table:: :docstring: pyopenms_viz._config.ChromatogramConfig :parent_depth: 1 - :default_docstring: + :title: Chromatogram Optionsdocs/Parameters/Mobilogram.rst (2)
4-4: Tighten wording.Minor style cleanup.
-Mobilograms are a type of plot used to visualize ion mobility data. In this plot, ion mobility is represented on the x-axis, while intensity is shown on the y-axis. The `by` parameter can be utilized to separate different mass traces. Functionally, mobilograms are similar to chromatograms. +Mobilograms visualize ion mobility data: ion mobility on the x-axis and intensity on the y-axis. The `by` parameter can be used to separate different mass traces. Functionally, mobilograms are similar to chromatograms.
9-12: Add a title and remove empty option.Keep the tables uniform across pages.
.. docstring_to_table:: :docstring: pyopenms_viz._config.MobilogramConfig :parent_depth: 1 - :default_docstring: + :title: Mobilogram Options
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/Parameters/Chromatogram.rst(1 hunks)docs/Parameters/Mobilogram.rst(1 hunks)docs/Parameters/Parameters.rst(1 hunks)docs/Parameters/PeakMap.rst(1 hunks)docs/Parameters/Spectrum.rst(1 hunks)docs/_ext/docstring_to_table.py(1 hunks)docs/conf.py(2 hunks)pyopenms_viz/_config.py(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
docs/_ext/docstring_to_table.py (1)
docs/conf.py (1)
setup(208-280)
🪛 Ruff (0.12.2)
docs/_ext/docstring_to_table.py
21-26: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
62-62: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: test (windows-latest, 3.12)
🔇 Additional comments (9)
docs/_ext/docstring_to_table.py (2)
85-95: Nice: required marker and child-overrides-base behavior are on point.Merging with child overriding parent and asterisking required params is a good UX.
Also applies to: 98-118
81-118: Ensure docstring_parser is declared in docs dependencies and “Attributes:” entries are handled
- No
docstring_parserfound in any requirements or lock file—add it (pinned) to your docs’ dependencies (e.g. pyproject.toml or docs/requirements.txt)- Verify your chosen version parses Google/Numpy
Attributes:intoparsed.params, or add a fallback to extract them fromparsed.metapyopenms_viz/_config.py (5)
313-335: Good: dict-to-LegendConfig conversion in post_init.Covers both dict and object inputs cleanly.
409-413: Good: defensive copy of reference_spectrum.Prevents aliasing bugs when mutating downstream.
460-478: Follow-up: guard after factory to ensure configs exist.If invalid kinds slip through, fail fast.
- # initial marginal configs - self.y_plot_config = PeakMapConfig.marginal_config_factory(self.y_kind) - self.x_plot_config = PeakMapConfig.marginal_config_factory(self.x_kind) + # initial marginal configs + self.y_plot_config = PeakMapConfig.marginal_config_factory(self.y_kind) + self.x_plot_config = PeakMapConfig.marginal_config_factory(self.x_kind) + assert self.y_plot_config and self.x_plot_config, "Invalid marginal kind"
503-551: Dash mapping utility looks solid.Reasonable defaults and coverage for both Plotly and Matplotlib.
44-57: No internal callers rely on silent‐ignore behavior
Scanned Config.update usages in pyopenms_viz/_config.py:27 and pyopenms_viz/_core.py:139; both only pass known fields. Raising on unknown keys won’t break any existing call sites.docs/conf.py (1)
103-104: Extension registration and sys.path configuration are correct. The verification script showssys.path[0]includes the repo root (empty string) anddocs/_extexists, so Read the Docs can importdocs._ext.docstring_to_table.docs/Parameters/PeakMap.rst (1)
11-14: Add table title and remove empty default_docstring
Replace the blank:default_docstring:with:title: Peak Map Options. The directivedocs._ext.docstring_to_tableis already registered indocs/conf.py(line 103) and its source file exists. Ensure thedocstring-parserpackage is listed in your docs requirements so the import indocs/_ext/docstring_to_table.pysucceeds, and confirm thatpyopenms_viz._config.PeakMapConfigis on the Sphinx Python path when building the docs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/_ext/docstring_to_table.py (1)
47-65: Fix dotted-path resolution; import the real module path and narrow exceptions.Current logic imports only the top-level package and getattr-walks submodules; this breaks for non-reexported submodules like pyopenms_viz._config.SpectrumConfig. Also avoids catching Exception broadly.
- # Split module and object - mod_name, _, obj_path = docstring_path.partition(".") - if not obj_path: - error = self.state_machine.reporter.error( - f"Invalid docstring path: {docstring_path}", line=self.lineno - ) - return [error] - - # Import module and get object - try: - mod = importlib.import_module(mod_name) - obj = mod - for attr in docstring_path.split(".")[1:]: - obj = getattr(obj, attr) - except Exception as e: - error = self.state_machine.reporter.error( - f"Could not import object '{docstring_path}': {e}", line=self.lineno - ) - return [error] + # Import module and resolve object (supports nested modules) + module_path, sep, attr_path = docstring_path.rpartition(".") + if not sep: + error = self.state_machine.reporter.error( + f"Invalid docstring path: {docstring_path}", line=self.lineno + ) + return [error] + try: + mod = importlib.import_module(module_path) + obj = mod + for attr in attr_path.split("."): + obj = getattr(obj, attr) + except (ImportError, AttributeError) as e: + error = self.state_machine.reporter.error( + f"Could not import object '{docstring_path}': {e}", line=self.lineno + ) + return [error]
🧹 Nitpick comments (5)
pyproject.toml (1)
34-34: Pin a minimum version for docstring_parser to avoid older-API breakage.Add a lower bound consistent with docs/requirements.txt to keep local/dev and CI environments aligned.
-docs = ["sphinx", "sphinx-gallery", "sphinx-copybutton", "pydata_sphinx_theme", "nbsphinx", "rdkit", "cairosvg", "pymzml", "ipython", "ipykernel", "docstring_parser"] +docs = ["sphinx", "sphinx-gallery", "sphinx-copybutton", "pydata_sphinx_theme", "nbsphinx", "rdkit", "cairosvg", "pymzml", "ipython", "ipykernel", "docstring_parser>=0.17"]docs/_ext/docstring_to_table.py (4)
1-6: Import typing for class-level annotations used below.from docutils.parsers.rst import Directive, directives from docutils import nodes import importlib import inspect import docstring_parser +from typing import Any, ClassVar
24-29: Annotate directive class attributes as ClassVar and use docutils converters.Addresses Ruff RUF012 and aligns with docutils option parsing.
-class DocstringToTableDirective(Directive): - has_content = False - required_arguments = 0 - optional_arguments = 0 - option_spec = { - "docstring": str, - "title": str, - "parent_depth": int, # Number of parent classes to include - "default_docstring": directives.flag, # Flag, no argument required - } +class DocstringToTableDirective(Directive): + has_content: ClassVar[bool] = False + required_arguments: ClassVar[int] = 0 + optional_arguments: ClassVar[int] = 0 + option_spec: ClassVar[dict[str, Any]] = { + "docstring": directives.unchanged, # presence validated below + "title": directives.unchanged, + "parent_depth": directives.nonnegative_int, # Number of parent classes to include + "default_docstring": directives.flag, # Flag, no argument required + }
67-79: Walk the MRO instead of a single base to respect inheritance order.Covers multiple inheritance and yields consistent base→derived merge with parent_depth.
- # Collect docstrings from parent classes up to parent_depth - docstrings = [] - current_obj = obj - for i in range(parent_depth + 1): - docstring = inspect.getdoc(current_obj) - if docstring: - docstrings.append(docstring) - bases = getattr(current_obj, "__bases__", ()) - if bases and i < parent_depth: - current_obj = bases[0] - else: - break + # Collect docstrings along MRO up to parent_depth + docstrings = [] + if inspect.isclass(obj): + mro = [c for c in inspect.getmro(obj) if c is not object][: parent_depth + 1] + else: + mro = [obj] + for cls in reversed(mro): # base -> derived + ds = inspect.getdoc(cls) + if ds: + docstrings.append(ds)
146-149: Return Sphinx extension metadata for parallel safety.Declare compatibility; harmless and useful for builders.
def setup(app): app.add_directive("docstring_to_table", DocstringToTableDirective) + return {"version": "1.0", "parallel_read_safe": True, "parallel_write_safe": True}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/_ext/docstring_to_table.py(1 hunks)docs/requirements.txt(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
docs/_ext/docstring_to_table.py (1)
docs/conf.py (1)
setup(208-280)
🪛 Ruff (0.12.2)
docs/_ext/docstring_to_table.py
24-29: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
61-61: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
docs/requirements.txt (1)
57-58: LGTM: docs dependency added for new directive.Pinning docstring-parser==0.17.0 matches the extension’s usage. No further changes needed.
docs/_ext/docstring_to_table.py (1)
80-117: Docstring_parser includes Attributes in params
Verified thatdocstring_parser.parse(v0.17.0) populates entries from an “Attributes:” section intop.params, so no additional handling is required.
|
📖 Documentation Preview The documentation for this PR has been built and is available at: This preview will be updated automatically when you push new commits to this PR. Preview built from commit: |
2 participants
Addresses #54
Background:
The parameters documentation section has quickly fallen out of date with the addition of new parameters.
This implementation scrapes the parameters from the _config,py docstrings to ensure that it always stays up to date.
One limitation is that this is not actually dependent on the class, it requires the _config.py docstrings to be updated manually.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation