Sync Config & Docs: Updated Parameters & Auto-Generated Docs #60
Conversation
…considering the updated config file
… documenation rendering
checking the peakMapPlot
Testing Check Mobilegram for table
WalkthroughA new Python script ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as update_tsv_docs.py
participant C as Config File
participant D as Dataclass Processor
participant T as TSV Handler
U->>S: Execute script
S->>C: Read configuration file
S->>D: Parse class definitions & inheritance
D-->>S: Return attributes & documentation info
S->>T: Check & read existing TSV files
S->>T: Update TSV content if changes detected
T-->>S: Write updated TSV files
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Nitpick comments (3)
Scripts/update_tsv_docs.py (3)
5-5: Remove unused import.The
osmodule is not referenced elsewhere and can be safely removed.-import os import re import ast import logging🧰 Tools
🪛 Ruff (0.8.2)
5-5:
osimported but unusedRemove unused import:
os(F401)
9-9: Remove unused import.
is_dataclassis never utilized in this file and should be removed to keep the code clean.-from dataclasses import is_dataclass from typing import Dict, List, Tuple from pathlib import Path🧰 Tools
🪛 Ruff (0.8.2)
9-9:
dataclasses.is_dataclassimported but unusedRemove unused import:
dataclasses.is_dataclass(F401)
89-91: Combine nested if statements.Consolidate the nested
ifconditions for readability.for node in parsed.body: - if isinstance(node, ast.ClassDef): - if any(d.id == 'dataclass' for d in node.decorator_list if isinstance(d, ast.Name)): - if node.name in CLASS_TO_TSV_MAP: - attrs = process_dataclass(node) + if (isinstance(node, ast.ClassDef) + and any(d.id == 'dataclass' for d in node.decorator_list if isinstance(d, ast.Name)) + and node.name in CLASS_TO_TSV_MAP): + attrs = process_dataclass(node)🧰 Tools
🪛 Ruff (0.8.2)
89-91: Use a single
ifstatement instead of nestedifstatements(SIM102)
90-91: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
docs/Parameters/basePlot.tsvis excluded by!**/*.tsvdocs/Parameters/chromatogramPlot.tsvis excluded by!**/*.tsvdocs/Parameters/mobilogramPlot.tsvis excluded by!**/*.tsvdocs/Parameters/peakMapPlot.tsvis excluded by!**/*.tsvdocs/Parameters/spectrumPlot.tsvis excluded by!**/*.tsv
📒 Files selected for processing (8)
Scripts/update_tsv_docs.py(1 hunks)docs/Parameters/Chromatogram.rst(1 hunks)docs/Parameters/Mobilogram.rst(2 hunks)docs/Parameters/PeakMap.rst(1 hunks)docs/Parameters/Spectrum.rst(1 hunks)docs/conf.py(2 hunks)docs/readthedocs.yml(1 hunks)pyopenms_viz/_config.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
Scripts/update_tsv_docs.py
5-5: os imported but unused
Remove unused import: os
(F401)
9-9: dataclasses.is_dataclass imported but unused
Remove unused import: dataclasses.is_dataclass
(F401)
89-91: Use a single if statement instead of nested if statements
(SIM102)
90-91: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (9)
pyopenms_viz/_config.py (1)
207-208: Whitespace Consistency ImprovedA new blank line has been inserted after the declaration of the
legend_configattribute in theBasePlotConfigclass. This addition enhances readability without altering functionality.docs/Parameters/Chromatogram.rst (1)
13-14: Improved Section SpacingThe insertion of blank lines before the "Example Usage" section creates a clearer visual break between the different parts of the document, improving overall readability.
docs/Parameters/PeakMap.rst (1)
16-17: Enhanced Readability Through SpacingAdding a blank line before the "Example Usage" section strengthens the separation between the "Parameters" and "Example Usage" sections. This formatting adjustment parallels similar improvements across other documentation files.
docs/readthedocs.yml (1)
1-12: New Read the Docs ConfigurationThis new configuration file correctly specifies the Read the Docs settings, including the Sphinx configuration file (
docs/conf.py), installation requirements fromrequirements.txt, and the designated Python version (3.8). This change will streamline the documentation build and deployment process.docs/Parameters/Spectrum.rst (2)
4-5: Grammar ImprovementThe descriptive sentence has been updated to use "can be plotted" instead of the previously incorrect phrasing. This correction clarifies the intended meaning and improves overall readability.
9-12: CSV Table Directive RefinementThe CSV table directive now omits a specific title (previously "Chromatogram Options"), standardizing the format across documentation files. Ensure that this change aligns with the overall documentation style.
docs/conf.py (1)
324-326: Ensure the new variable is used or remove it if unnecessary.
spectrum_peak_count = 500appears to be a new configuration variable. Verify that it is used in the documentation or code paths to avoid confusion or stale config values.docs/Parameters/Mobilogram.rst (2)
4-4: Clear and concise documentation.Thanks for expanding the explanation on mobilograms. The updated text accurately clarifies their purpose and usage.
17-17: Example usage is well-provided.Including the minigallery scripts helps users quickly see how to generate mobilogram plots. Good addition!
docs/conf.py
Outdated
| "sphinx.ext.autosectionlabel", | ||
| "sphinx.ext.githubpages", | ||
| "sphinx_copybutton", | ||
| "sphinx.ext.csv", # Add this line |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify extension availability.
The extension "sphinx.ext.csv" is not a standard built-in Sphinx extension. Ensure it is installed or recognized in your environment, or replace it with an equivalent extension if necessary.
🌐 Web query:
Is "sphinx.ext.csv" a built-in or third-party extension for Sphinx?
💡 Result:
The sphinx.ext.csv extension is not a built-in Sphinx extension. It is a third-party extension developed by Crate.IO, as evidenced by:
-
Installation Requirements:
The extension must be installed via pip (pip install sphinx-csv-filter) and explicitly added to Sphinx'sextensionslist inconf.pyascrate.sphinx.csv[1]. Built-in extensions (e.g.,sphinx.ext.autodoc) do not require installation. -
Functionality:
It adds a customcsv-filterdirective to enhance CSV table handling with row filtering and column inclusion/exclusion options, which are not part of Sphinx's core features[1]. -
Documentation:
Official Sphinx documentation[3][4] lists built-in extensions (prefixed withsphinx.ext.*), andsphinx.ext.csvis absent. Third-party extensions like this one are hosted separately (e.g., on GitHub)[1][2].
For comparison, built-in CSV handling in Sphinx uses the standard csv-table directive[8][16], while this extension adds advanced filtering capabilities.
Citations:
- 1: https://github.com/crate/sphinx_csv_filter
- 2: http://zh-sphinx-doc.readthedocs.io/en/latest/extensions.html
- 3: https://www.sphinx-doc.org/en/master/usage/configuration.html
- 4: https://www.sphinx-doc.org/en/master/usage/extensions/index.html
- 5: https://sphinx-intro-tutorial.readthedocs.io/en/latest/sphinx_extensions.html
- 6: https://sammart.in/post/2021-05-10-external-data-sphinx-extension/
- 7: https://sharm294.github.io/sphinx-datatables/
- 8: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html
- 9: https://www.ericholscher.com/blog/2016/jul/1/sphinx-and-rtd-for-writers/
- 10: https://sphinx-tutorial.readthedocs.io/finish/
- 11: Absolute/"source-relative" imports for csv-table :file: sphinx-doc/sphinx#8487
- 12: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html
- 13: https://pandas.pydata.org/docs/user_guide/io.html
- 14: https://docs.getodk.org/docs-syntax-guide/
- 15: https://media.readthedocs.org/pdf/sphinx/master/sphinx.pdf
- 16: https://sublime-and-sphinx-guide.readthedocs.io/en/latest/tables.html
- 17: Allow unqualified type hint names in generated docs sphinx-doc/sphinx#7119
CSV Extension Configuration Correction
In docs/conf.py (line 99), the extension entry "sphinx.ext.csv" is incorrect because it is not a built-in Sphinx extension. This third-party extension is developed by Crate.IO and must be installed via pip install sphinx-csv-filter. Additionally, you should update the extension name to "crate.sphinx.csv" in the configuration.
- Install using:
pip install sphinx-csv-filter - Update the config entry in
docs/conf.pyfrom"sphinx.ext.csv"to"crate.sphinx.csv" - Alternatively, if advanced CSV filtering isn’t required, consider using the built-in
csv-tabledirective
There was a problem hiding this comment.
Done ! I have removed the line sphinx.ext.csv and check by rendering again
There was a problem hiding this comment.
@Imama-Kainat Great job! Removing the sphinx.ext.csv extension was the right approach. Sphinx already has built-in CSV table support through the csv-table directive, so you don't need any additional extensions for basic CSV functionality. Your documentation should render correctly now.
If you need to work with CSV data in your documentation in the future, you can use the standard directive like this:
.. csv-table:: Title
:header: "Column 1", "Column 2", "Column 3"
:widths: 15, 10, 30
"data1", "data2", "data3"
"data4", "data5", "data6"✏️ Learnings added
Learnt from: Imama-Kainat
PR: OpenMS/pyopenms_viz#60
File: docs/conf.py:0-0
Timestamp: 2025-03-08T18:00:46.168Z
Learning: Sphinx has built-in CSV table support through the `csv-table` directive without requiring any additional extensions. The `sphinx.ext.csv` is not a standard extension and should not be added to the configuration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
AutoGenerated Docs Check✅ |
|
@jcharkow @t0mdavid-m Please review and let me know if any modifications are required. |
There was a problem hiding this comment.
Great job so far as this is one of the more challenging PRs.
I have not had the chance to test your script yet but I trust that it is working as intended. One thing I did notice though is that it seems that the inheritance might not be followed correctly in your script as some parameters seem to be missing.
Also the ordering of some parameters is important. It is important that x, y, z, by, and backend are at the top because these are basic parameters that would be commonly modified. x, y (and possibly z) should have an "*" next to them to indicate that they are mandatory. It might be a good idea to just hardcode these in your script.
Something to consider is whether it is worth encoding the data structures with
x = field(str, metadata=dict(description="description of variable")
as this might be easier to parse than the docstrings however it seems that you are parsing the docstrings well already so this might not be necessary.
Please see comments above as I also have a few other minor comments.
docs/readthedocs.yml
Outdated
| version: 2 | ||
|
|
||
| sphinx: | ||
| configuration: docs/conf.py | ||
|
|
||
| python: | ||
| install: | ||
| - requirements: requirements.txt | ||
| version: 3.8 | ||
|
|
||
| # Optional: Add any additional configuration here |
There was a problem hiding this comment.
There is already a .readthedocs.yml file in the main directory so this is likely unneeded.
There was a problem hiding this comment.
I have delete it
|
|
||
|
|
|
Acknowlege ! |
|
@Imama-Kainat please let me know when this is ready for re-review. I see that you have acknowledged all of the comments but I don't see any new commits |
…ance, and Git Integration **Issues Resolved:** ✅ **Fixed attribute extraction from _config.py** – Now correctly retrieves all attributes, including inherited ones. ✅ **Handled missing attributes in dataclasses** – Ensured that no attributes are skipped when updating .tsv files. ✅ **Resolved file writing issues** – .tsv files now update only when necessary, preventing unnecessary commits. ✅ **Preserved existing descriptions in .tsv files** – Ensures no loss of documentation during updates. ✅ **Integrated with Git pre-commit hook** – Automatically stages updated .tsv files before commit. ✅ **Fixed Git detection of modified .tsv files** – Git now correctly recognizes changes to documentation files. 📌 **Key Changes:** - Updated update_tsv_docs.py to correctly extract attributes and inheritance. - Improved logging for better debugging and visibility of updates. - Fixed issues where .tsv files were not updating due to skipped attributes. - Enhanced pre-commit hook to detect and stage .tsv changes automatically.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Scripts/update_tsv_docs.py (2)
12-12: Remove unused typing importsThe
Listtype is imported but never used in the code. Consider removing it to keep imports clean.-from typing import Dict, List, Tuple +from typing import Dict, Tuple🧰 Tools
🪛 Ruff (0.8.2)
12-12:
typing.Listimported but unusedRemove unused import
(F401)
12-12:
typing.Tupleimported but unusedRemove unused import
(F401)
90-90: Remove extra blank lineThere's an unnecessary blank line that can be removed.
return attributes - def update_tsv_files():
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
docs/Parameters/basePlot.tsvis excluded by!**/*.tsvdocs/Parameters/chromatogramPlot.tsvis excluded by!**/*.tsvdocs/Parameters/mobilogramPlot.tsvis excluded by!**/*.tsvdocs/Parameters/peakMapPlot.tsvis excluded by!**/*.tsvdocs/Parameters/spectrumPlot.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
.gitignore(1 hunks)Scripts/update_tsv_docs.py(1 hunks)docs/Parameters/Mobilogram.rst(1 hunks)docs/Parameters/Spectrum.rst(1 hunks)pyopenms_viz/_config.py(0 hunks)
💤 Files with no reviewable changes (1)
- pyopenms_viz/_config.py
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/Parameters/Spectrum.rst
- docs/Parameters/Mobilogram.rst
🧰 Additional context used
🪛 Ruff (0.8.2)
Scripts/update_tsv_docs.py
7-7: os imported but unused
Remove unused import: os
(F401)
12-12: typing.List imported but unused
Remove unused import
(F401)
12-12: typing.Tuple imported but unused
Remove unused import
(F401)
🔇 Additional comments (7)
Scripts/update_tsv_docs.py (7)
7-7: Remove unused importThe
osmodule is imported but never used in the code. This import can be safely removed to keep the codebase clean.-import os🧰 Tools
🪛 Ruff (0.8.2)
7-7:
osimported but unusedRemove unused import:
os(F401)
30-35: Good implementation of type normalizationThe type normalization function effectively simplifies complex type annotations using regular expressions, making the output more readable in the TSV files.
37-51: Well-structured docstring parserThis function correctly extracts parameter descriptions from Sphinx-style docstrings and handles multi-line descriptions appropriately.
53-89: Solid implementation of attribute inheritanceThe recursive attribute retrieval function correctly handles inheritance hierarchies while properly skipping abstract base classes. The logging statements provide good visibility into the process.
91-156: Well-implemented TSV update functionThe main function correctly:
- Extracts class definitions and inheritance relationships
- Processes each configured class
- Preserves existing descriptions when available
- Only writes files when content has changed
- Provides appropriate error handling
This implementation aligns well with the PR objectives of automating documentation updates.
158-159: LGTM!The main execution block is properly implemented.
1-160: Overall excellent script implementationThis script successfully fulfills the PR objective of automating documentation updates. It handles class inheritance properly, normalizes type annotations, and preserves existing descriptions. The error handling is robust, and the logging provides good visibility into the process.
A few minor suggestions above for improvement, but overall this is a well-written script that will help maintain documentation consistency.
🧰 Tools
🪛 Ruff (0.8.2)
7-7:
osimported but unusedRemove unused import:
os(F401)
12-12:
typing.Listimported but unusedRemove unused import
(F401)
12-12:
typing.Tupleimported but unusedRemove unused import
(F401)
|
Hey @jcharkow , Apologies for the delayed response, and thanks for your detailed feedback! 😊Actually I went through all your comments and made the necessary changes, but your one comment about inheritance really shifted my perspective. Initially, the script was working perfectly because each class was independent and had the @DataClass decorator directly applied. However, after introducing inheritance, some classes are no longer being detected correctly. Here’s what I’ve observed: 1️⃣ Previously, all classes were processed because they had @DataClass applied directly. What I Tried So Far: Despite these fixes, something still seems off. I’m sharing a diagram of the inheritance structure along with the current script format to provide more clarity. I’ll be stepping away for a bit and will revisit with fresh eyes after reviewing other PRs. If you have any insights on what might be missing, I’d really appreciate it! Thanks for your patience, and I’ll refine this further soon. 😊 |
|
|
|
My current Script flow |
In this update, I streamlined the documentation update process by integrating automation for detecting configuration changes and updating TSV files accordingly.
📌 Branch: ParameterDocUpdation
Here's what I did:
1️⃣ Updated TSV Files: Refactored the tables in the documentation to match the latest changes in the config file. This ensures that the documentation stays accurate and up to date.
2️⃣ Testing & Verification: I checked the updates by reading the rendered docs to confirm that everything displays correctly after the changes.
3️⃣ Automation Script:
Scripts/folder and addedupdate_tsv_docs.pyto automatically detect config file changes.4️⃣ Git Pre-Commit Hook:
_config.pyis modified.5️⃣ Final Testing:
Files Added:
✅
.git/hooks/pre-commit→ Ensures automatic documentation updates before every commit.✅
Scripts/update_tsv_docs.py→ Detects config changes and updates TSV files accordingly.What This Script Can Handle?
_config.pydocs/Parameters/This approach removes the need for manual TSV updates, making the process smoother and error-free. 🚀
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores