fix: linting/formatting results in scrollable window, more cross refs #48
Conversation
📝 Walkthrough""" WalkthroughThe updates modify documentation templates, workflow badge references, and output display for quality checks. The README's deployment badge is updated, Sphinx documentation configuration is changed to enable heading anchors, and the function for truncating quality check outputs now allows a configurable line limit. No public APIs or logic are otherwise altered. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sphinx
participant Template
participant build_wf_pages.py
User->>Sphinx: Build documentation
Sphinx->>Template: Render workflow page
Template->>build_wf_pages.py: Call check_qc_output(qc_item, max_lines=200)
build_wf_pages.py-->>Template: Return truncated QC output
Template-->>Sphinx: Rendered page with enhanced badges and output display
Sphinx-->>User: Serve documentation with updated badges and headings
Suggested reviewers
✨ 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: 4
🧹 Nitpick comments (2)
source/_templates/workflow_page.md (2)
147-156: Refactor inline scroll container stylingInline styles reduce maintainability and consistency. Extract the scrollable container styles (height, overflow, padding) into a CSS class in your Sphinx theme or custom CSS file. Also, remove the trailing space in the closing tag (
</div >→</div>).
167-176: Extract inline styles for formatting results containerSimilarly, move the inline scroll-container styling into a dedicated CSS class and correct the closing tag (
</div >→</div>). This will keep the template cleaner and consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)source/_templates/all_standardized_workflows.md(1 hunks)source/_templates/workflow_page.md(2 hunks)source/build_wf_pages.py(1 hunks)source/conf.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
source/conf.pysource/build_wf_pages.py
🪛 markdownlint-cli2 (0.17.2)
source/_templates/workflow_page.md
143-143: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-catalog
🔇 Additional comments (5)
README.md (1)
4-4:✅ Verification successful
Verify the updated workflow name exists.
The badge update looks correct, but please ensure the "deploy" workflow exists in the repository.
🏁 Script executed:
#!/bin/bash # Description: Verify the deploy workflow exists in the repository # Expected: The deploy.yml workflow file should exist fd -t f "deploy.yml" .github/workflows/Length of output: 68
“deploy” workflow badge validated
The file
.github/workflows/deploy.ymlexists, so the badge now correctly points to a valid workflow. Approving the update.source/conf.py (1)
61-61: LGTM: Enables heading anchors for improved navigation.This setting correctly enables automatic anchor generation for headings, supporting the cross-reference improvements mentioned in the PR objectives.
source/_templates/all_standardized_workflows.md (1)
25-25:✅ Verification successful
Verify anchor targets exist in workflow pages.
The cross-reference implementation looks correct, converting simple badges to clickable reference badges.
🏁 Script executed:
#!/bin/bash # Description: Verify that corresponding anchor targets exist in workflow page templates # Expected: Should find anchor definitions matching the reference format rg -A 3 -B 3 "formatting-|linting-" source/_templates/workflow_page.mdLength of output: 670
Cross-reference anchor targets confirmed.
Verified in
source/_templates/workflow_page.mdthat the anchors
(linting-{{ wf["full_name"] }})(formatting-{{ wf["full_name"] }})exist and match the reference badges in
all_standardized_workflows.md. Approving these changes.source/build_wf_pages.py (2)
29-29: LGTM: Configurable line limit improves flexibility.The addition of the
max_linesparameter with a default of 200 is a good improvement that maintains backward compatibility while allowing more content to be displayed.
34-35: LGTM: Proper implementation of configurable truncation.The truncation logic correctly uses the new
max_linesparameter instead of the hardcoded value, supporting the PR objective of showing more content in scrollable containers.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 34-37: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
|
@m-jahn I will be on a conference for the next week. Also, I am not familiar with the code base in the first place. Still, it looks good at first glance. Mind addressing the coderabbit's comments (I sometimes outright dismiss them, as they are bullshit; sometimes they are really helpful because I missed something). NB I would not know how to test the web setup. |
|
@cmeesters yes you're right, the code rabbit comments can be helpful, I try to address them. Regarding how to test the catalog locally, there is a new section on the README which describes the steps needed to build the catalog and test-deploy the web page. These are exactly the steps that I use to test changes. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
source/_templates/workflow_page.md (2)
35-35: Excellent resolution of anchor sanitization issues.The badges now correctly use the
slugifyfilter to generate safe anchor references, addressing the previous concerns about invalid characters inwf["full_name"]breaking HTML anchors.Also applies to: 41-41
139-139: Properly slugified section anchors match badge references.The section anchors consistently use slugified workflow names, ensuring they properly match the badge references and maintain valid HTML anchor syntax.
Also applies to: 159-159
🧹 Nitpick comments (1)
source/build_wf_pages.py (1)
31-38: Simplify by removing unnecessary else clauses.The
elseclauses afterreturnstatements are unnecessary and can be removed for cleaner code.def check_qc_output(qc_item, max_lines=200): if qc_item is None: return None - else: - result = qc_item.split("\n") - if len(result) > max_lines: - return "\n".join(result[:max_lines]) + "\n\n... (truncated)" - else: - return "\n".join(result) + + result = qc_item.split("\n") + if len(result) > max_lines: + return "\n".join(result[:max_lines]) + "\n\n... (truncated)" + return "\n".join(result)🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 31-38: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 35-38: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
source/_templates/all_standardized_workflows.md(1 hunks)source/_templates/workflow_page.md(2 hunks)source/build_wf_pages.py(3 hunks)source/build_wf_tables.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/_templates/all_standardized_workflows.md
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
source/build_wf_tables.pysource/build_wf_pages.py
🧬 Code Graph Analysis (2)
source/build_wf_tables.py (1)
source/build_wf_pages.py (1)
slugify(41-45)
source/build_wf_pages.py (1)
source/build_wf_tables.py (1)
slugify(19-23)
🪛 markdownlint-cli2 (0.17.2)
source/_templates/workflow_page.md
143-143: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 Pylint (3.3.7)
source/build_wf_pages.py
[refactor] 31-38: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 35-38: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-catalog
🔇 Additional comments (8)
source/build_wf_tables.py (3)
2-2: Import added for slugify functionality.The
remodule import is appropriately added to support the newslugifyfunction.
19-23: Well-implemented slugify function.The
slugifyfunction correctly converts strings to URL-friendly format with proper handling of edge cases (trimming leading/trailing hyphens).
101-101: Slugify filter registration enables template functionality.Correctly registers the
slugifyfunction as a Jinja2 filter, enabling its use in templates via the|slugifysyntax.source/build_wf_pages.py (4)
2-2: Import added for slugify functionality.The
remodule import supports the newslugifyfunction implementation.
30-30: Improved configurability for output truncation.Great improvement! The
max_linesparameter makes the truncation limit configurable (defaulting to 200 lines vs. the previous hardcoded 20), providing much more useful output.
41-45: Consistent slugify implementation across files.The
slugifyfunction is identical to the one inbuild_wf_tables.py, ensuring consistent slug generation behavior across the codebase.
56-56: Slugify filter enables template anchor generation.Properly registers the
slugifyfunction as a Jinja2 filter for template use, supporting the anchor generation functionality in workflow templates.source/_templates/workflow_page.md (1)
147-156: Enhanced output display with scrollable containers.Great UX improvement! The scrollable containers with fixed height (400px) and line numbers provide much better presentation of long linting/formatting results compared to the previous truncated approach.
Also applies to: 167-176
Summary by CodeRabbit