Refactor and cleanup of documentation making code #544
Conversation
- Fix Markdown generation for user documentation from JSON. - Ensure correct placement of metadata (Status, Author, Maintainer). - Prevent category headers from incorrectly displaying metadata. - Small improvements to path handling and formatting. - Slightly sharper structure AI assisted
|
Warning Rate limit exceeded@igorpecovnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (18)
WalkthroughThis set of changes encompasses significant updates across Python tooling scripts, JSON configuration files, and Markdown documentation assets. The main Python script, The JSON configuration files for network, system, and software menus underwent widespread renaming of menu entry IDs to new, more descriptive or categorized codes. Numerous entries received new "short" fields for concise labeling, and some descriptions were adjusted for consistency. No structural or command logic changes were made to these configurations. A substantial number of Markdown files were added, removed, or updated under the ✨ 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:
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 (
|
github-actions
bot
added
05
igorpecovnik
added
Work in progress
![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 (1)
tools/config-markdown.py (1)
1-168: Overall excellent modernization with one critical issueThis refactoring greatly improves the code quality by:
- Using
pathlib.Pathconsistently for modern path operations- Implementing list-based Markdown construction instead of string concatenation
- Improving error handling and metadata display logic
- Adding better docstrings and clearer variable names
However, there's one critical issue with the command loop (lines 100-102) that needs to be fixed before merging to ensure commands are properly displayed.
Consider adding type hints to the function parameters and return values for better code clarity and IDE support. For example:
def create_markdown_user(item: dict, level: int = 1, show_meta: bool = True) -> str:🧰 Tools
🪛 Ruff (0.8.2)
3-3:
osimported but unusedRemove unused import:
os(F401)
101-101: Loop control variable
cmdnot used within loop bodyRename unused
cmdto_cmd(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/config-markdown.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tools/config-markdown.py
101-101: Loop control variable cmd not used within loop body
Rename unused cmd to _cmd
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Coding style check
- GitHub Check: Validate JSON
- GitHub Check: Armbian configurator unit tests
🔇 Additional comments (7)
tools/config-markdown.py (7)
5-14: Good improvement in path handling with pathlibThe switch from
os.pathtopathlib.Pathis a welcome modernization that makes the path operations more readable and maintainable. Using constants with descriptive names improves clarity.
16-23: Clean error handling with Path.exists()Good refactoring of the file existence check to use
Path.exists()method, consistent with the pathlib approach. The error message is clear and helpful.
26-36: Improved docstring and variable clarityThe updated docstring provides better context, and the code is now more readable with clearer variable usage.
38-62: Excellent refactoring to list-based Markdown constructionThe switch from string concatenation to a list-based approach with final joining is a best practice that improves both readability and performance. The use of
dict.get()for safer access to dictionary keys is also a good defensive programming technique.
64-112: Significantly improved user documentation generationThe complete rewrite of this function is excellent, addressing the PR's goal of fixing Markdown generation for proper placement of metadata fields. The conditional logic for showing metadata only at appropriate levels is a key improvement.
🧰 Tools
🪛 Ruff (0.8.2)
101-101: Loop control variable
cmdnot used within loop bodyRename unused
cmdto_cmd(B007)
115-148: Streamlined file operations with pathlibGood consistent use of pathlib throughout the file writing functions. The
Path.mkdir(exist_ok=True)andPath.write_text()methods simplify the code compared to the older os module approach.
158-164: Improved user interface messagesThe output messages have been simplified, and the usage instructions are now formatted more clearly with separate lines. This makes the CLI more user-friendly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/json/config.system.json (1)
568-568: Add trailing newlineThe JSON file currently ends without a final newline. While JSON parsers will handle this, adding an end-of-file newline is a POSIX best practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/json/config.network.json(10 hunks)tools/json/config.system.json(43 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/json/config.network.json
🔇 Additional comments (5)
tools/json/config.system.json (5)
80-224: VerifySTORandNFSprefixes under StorageStorage menu items now use
STOR001–STOR005, and Network File System entries under Storage correctly useNFS006–NFS013. The ID ranges are unique, sequential, and aligned with the documented prefix scheme.
232-400: Access sub-menu usesACCprefix correctlyAll SSH/Access-related items have been updated to
ACC001throughACC015. The renaming is consistent across commands and conditions, with no duplicates detected.
408-437: User sub-menu usesUSRprefix correctlyUser management entries have been standardized to
USR001–USR003, matching the new prefix guidelines. This enhances uniformity across the menu configuration.
448-562: Updates sub-menu usesUPDprefix correctlyAll update-related IDs now range from
UPD001toUPD011, in clear sequential order. This follows the category-specific prefix pattern and removes previous inconsistencies.
12-71:✅ Verification successful
Consistent
KERprefix for Kernel sub-itemsAll kernel-related menu entries have been renamed to
KER001throughKER006, adhering to the new category-specific prefix convention and improving clarity.Please verify that no legacy
SY-prefixed IDs remain referenced in related JSON configurations or code. You can run:
🏁 Script executed:
#!/bin/bash # Check for obsolete 'SY' prefix in config JSON files rg -n '"SY[0-9]{3}"' tools/jsonLength of output: 33
🏁 Script executed:
#!/bin/bash # Double-check for any legacy 'SY' prefixes across the entire repository rg -n '"SY[0-9]{3}"'Length of output: 278
Legacy SY prefix verification complete
A global search for"SY[0-9]{3}"only returned two valid System→Storage entries (SY220, SY221) intools/modules/runtime/config.runtime.sh. No legacy SY prefixes remain in kernel-related JSON or code—all kernel items correctly useKER001–KER006.
github-actions
bot
removed
the
Ready to merge
Docstrings generation was requested by @igorpecovnik. * #544 (comment) The following files were modified: * `tools/config-markdown.py`
|
Note Generated docstrings for this pull request at #545 |
Docstrings generation was requested by @igorpecovnik. * #544 (comment) The following files were modified: * `tools/config-markdown.py`
github-actions
bot
removed
the
Ready to merge
|
@coderabbitai can you make a list of ID changes |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@igorpecovnik Here's a comprehensive list of all ID changes in the pull request: Network Configuration ID Changes (tools/json/config.network.json)
System Configuration ID Changes (tools/json/config.system.json)Kernel Section
Storage Section
Access Section
User Section
Updates Section
The ID naming has been standardized to use prefixes that reflect each category: NET for Network, KER for Kernel, STOR for Storage, NFS for Network File System, ACC for Access, USR for User, UPD for Updates, BT for Bluetooth, and IPV for IP version settings. (_/) 🏁 Script executed: Length of output: 676 🏁 Script executed: Length of output: 5962 @igorpecovnik Here's a comprehensive list of all ID changes in the pull request: Network Configuration ID Changes (config.network.json)
System Configuration ID Changes (config.system.json)Kernel Section
Storage Section
Network File System Section
Access Section
User Section
Updates Section
*Note: SY101 and SY102 appear in both the Access and Updates sections with different new IDs. The ID naming has been standardized to use more descriptive prefixes for each category:
|
igorpecovnik
added
Needs review
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/config-markdown.py (1)
127-129:⚠️ Potential issueFix unused loop variable in command iteration
The loop iterates over each command in
item.get('command'), but doesn't use thecmdvariable in the loop body. This means the same command is repeatedly printed for each item in the command list.Apply this fix to properly use each command:
# Commands if item.get('command'): for cmd in item['command']: - md.append(f"\n~~~ bash title=\"{item.get('description', '')}:\"\narmbian-config --cmd {item['id']}\n~~~\n") + md.append(f"\n~~~ bash title=\"{item.get('description', '')}:\"\n{cmd}\n~~~\n")🧰 Tools
🪛 Ruff (0.8.2)
128-128: Loop control variable
cmdnot used within loop bodyRename unused
cmdto_cmd(B007)
🧹 Nitpick comments (3)
tools/config-markdown.py (3)
22-23: Consider using Path.read_text() for consistencyWhile this approach works, for consistency with other parts of the code that use pathlib methods, consider:
-with open(CONFIG_PATH, 'r') as f: - data = json.load(f) +data = json.loads(CONFIG_PATH.read_text())
176-177: Consider parameterizing YAML front matterThe YAML front matter is duplicated across several places in the code. Consider extracting it to a constant to maintain consistency and make future changes easier.
+# Define common YAML front matter +YAML_FRONT_MATTER = '---\ncomments: true\n---\n\n' def write_user_markdown_files(data): # ... - (item_dir / f"{item['id']}.md").write_text('---\ncomments: true\n---\n\n' + user_md) + (item_dir / f"{item['id']}.md").write_text(YAML_FRONT_MATTER + user_md)
1-208: Overall excellent refactoring with modern Python practicesThe refactoring of this script is excellent overall. It uses modern Python practices like pathlib for file operations, builds strings efficiently using lists with join, provides comprehensive docstrings, and simplifies error handling. These changes align well with the PR objective to improve the documentation generation process.
The only critical issue is the unused loop variable in the command iteration which should be fixed. Consider also adding error handling for file operations and possibly extracting repeated code patterns into helper functions in future iterations.
🧰 Tools
🪛 Ruff (0.8.2)
3-3:
osimported but unusedRemove unused import:
os(F401)
128-128: Loop control variable
cmdnot used within loop bodyRename unused
cmdto_cmd(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/config-markdown.py(1 hunks)tools/json/config.system.json(43 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/json/config.system.json
🧰 Additional context used
🪛 Ruff (0.8.2)
tools/config-markdown.py
128-128: Loop control variable cmd not used within loop body
Rename unused cmd to _cmd
(B007)
🔇 Additional comments (6)
tools/config-markdown.py (6)
9-14: Clean path handling with pathlib.PathGood conversion from string-based path handling to using the modern
pathlib.Pathlibrary. This makes the code more readable and less error-prone by removing manual path concatenation and string manipulation.
17-19: Improved error handlingThe error message now provides more helpful information by suggesting the user run 'config_assemble.sh -p' or '-t' first when the configuration file is not found.
42-78: Good improvement to Markdown generation functionThe refactoring of
create_markdown_technicalto use list-based Markdown generation instead of string concatenation is more efficient and easier to maintain. The detailed docstrings also make the function's purpose and parameters clearer.
93-138: Well-structured user Markdown generation with conditional metadataThe rewritten
create_markdown_userfunction is well organized with clear separation between different content sections (title, metadata, images, etc.). The selective display of metadata (show_metaparameter) prevents metadata from incorrectly appearing in category headers, which was likely one of the issues being addressed.🧰 Tools
🪛 Ruff (0.8.2)
128-128: Loop control variable
cmdnot used within loop bodyRename unused
cmdto_cmd(B007)
147-157: Excellent simplification of file operationsGreat use of
Path.mkdir(exist_ok=True)instead of explicit directory existence checks, andPath.write_text()instead of manually opening and writing to files. This makes the code more concise and less error-prone.
184-205: Better documentation and clearer usage instructionsThe main function now has a comprehensive docstring and the usage instructions are clearer with the options on separate lines. This makes the script more user-friendly.
github-actions
bot
removed
the
Ready to merge
github-actions
bot
removed
the
Ready to merge
igorpecovnik
force-pushed
the
improve_docs_gen
branch
16 times, most recently
from
2ca06fd to
a8db606
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tools/config-markdown.py (4)
73-77:⚠️ Potential issueFix command handling in main items
Similar to the sub-items issue in previous reviews, this code only uses the first command if it's a list, and doesn't use the actual command value.
- if item.get('command') and not skip_commands: - cmd = item['command'][0] if isinstance(item['command'], list) else item['command'] - fence = "custombash" - title = "" - md.append(f"\n~~~ {fence}{title}\narmbian-config --cmd {item['id']}\n~~~\n") + if item.get('command') and not skip_commands: + commands = item['command'] if isinstance(item['command'], list) else [item['command']] + fence = "custombash" + title = "" + for command in commands: + md.append(f"\n~~~ {fence}{title}\n{command}\n~~~\n")
92-124:⚠️ Potential issueEnhance sub-item processing with proper command handling
There are two issues that need fixing:
- The
prefixvariable in the outer loop (line 92) is not used effectively- The same command issue occurs again in the inner loop (line 109-112)
- for prefix, sub_items in grouped_subs.items(): + for prefix, sub_items in grouped_subs.items(): + # Add a section header for the prefix group if desired + # md.append(f"\n### {prefix} Settings\n") first_sub = True first_command = True for sub_item in sub_items: if first_sub: header_prefix = "#" * (level + 1) md.append(f"{header_prefix} {sub_item.get('short', sub_item.get('description', ''))}\n") if sub_item.get('short') and sub_item.get('description') and sub_item.get('short') != sub_item.get('description'): md.append(f"\n{sub_item.get('description')}\n") md.extend(insert_images_and_header(sub_item)) if sub_item.get('author'): md.append(f"**Author:** {sub_item['author']}\n") if sub_item.get('status'): md.append(f"**Status:** {sub_item['status']}\n") first_sub = False - if sub_item.get('command'): - cmd = sub_item['command'][0] if isinstance(sub_item['command'], list) else sub_item['command'] - fence = "custombash" if first_command else "bash" - title = "" if fence == "custombash" else f" title=\"{sub_item.get('short', sub_item.get('description', ''))}:\"" - md.append(f"\n~~~ {fence}{title}\narmbian-config --cmd {sub_item['id']}\n~~~\n") - first_command = False + if sub_item.get('command'): + commands = sub_item['command'] if isinstance(sub_item['command'], list) else [sub_item['command']] + for command in commands: + fence = "custombash" if first_command else "bash" + title = "" if fence == "custombash" else f" title=\"{sub_item.get('short', sub_item.get('description', ''))}:\"" + md.append(f"\n~~~ {fence}{title}\n{command}\n~~~\n") + first_command = False🧰 Tools
🪛 Ruff (0.8.2)
92-92: Loop control variable
prefixnot used within loop body(B007)
109-109: Local variable
cmdis assigned to but never usedRemove assignment to unused variable
cmd(F841)
137-137:⚠️ Potential issueAddress undefined function 'create_markdown_technical'
The function
create_markdown_technicalis called but not defined anywhere in the file. This will cause runtime errors when attempting to generate technical documentation.#!/bin/bash # Check if create_markdown_technical is defined elsewhere grep -r "def create_markdown_technical" --include="*.py" .🧰 Tools
🪛 Ruff (0.8.2)
137-137: Undefined name
create_markdown_technical(F821)
143-145:⚠️ Potential issueFix unused variable and undefined function
Two issues need to be addressed:
- The variable
sub_anchorsis assigned but never used- The function
create_markdown_technicalis called again but not defined- sub_anchors = "\n".join(generate_anchor_links(sub_item)) + "\n\n" sub_technical_md = create_markdown_technical(sub_item) (item_dir / f"{sub_item['id']}.technical.md").write_text('---\ncomments: true\n---\n\n' + sub_technical_md)🧰 Tools
🪛 Ruff (0.8.2)
143-143: Local variable
sub_anchorsis assigned to but never usedRemove assignment to unused variable
sub_anchors(F841)
144-144: Undefined name
create_markdown_technical(F821)
🧹 Nitpick comments (1)
tools/include/markdown/ROLLIN-header.md (1)
1-1: Add missing comma for grammatical correctnessThere should be a comma after "improvements" in the sentence to maintain proper grammar with coordinating conjunctions.
-The daily rolling repository offers frequently updated packages directly from development branches. It provides access to the latest features, bug fixes, and hardware support improvements but may introduce instability or regressions. This channel is intended for testing, development, and users who need the newest updates at the cost of reduced stability. +The daily rolling repository offers frequently updated packages directly from development branches. It provides access to the latest features, bug fixes, and hardware support improvements, but may introduce instability or regressions. This channel is intended for testing, development, and users who need the newest updates at the cost of reduced stability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: Possible missing comma found.
Context: ...fixes, and hardware support improvements but may introduce instability or regression...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
tools/include/images/BOOT01.pngis excluded by!**/*.pngtools/include/images/DTO001.pngis excluded by!**/*.pngtools/include/images/HEAD01.pngis excluded by!**/*.pngtools/include/images/SHELL1.pngis excluded by!**/*.pngtools/include/images/SSH001.pngis excluded by!**/*.pngtools/include/images/ZFS001.pngis excluded by!**/*.png
📒 Files selected for processing (16)
tools/config-markdown.py(2 hunks)tools/include/markdown/ACC001-header.md(1 hunks)tools/include/markdown/BOOT01-header.md(1 hunks)tools/include/markdown/DTO001-header.md(1 hunks)tools/include/markdown/HEAD01-header.md(1 hunks)tools/include/markdown/KER001-header.md(1 hunks)tools/include/markdown/NETFS01-header.md(1 hunks)tools/include/markdown/ROLLIN-header.md(1 hunks)tools/include/markdown/ROO001-header.md(1 hunks)tools/include/markdown/SHELL1-header.md(1 hunks)tools/include/markdown/STABLE-header.md(1 hunks)tools/include/markdown/STO001-header.md(1 hunks)tools/include/markdown/UPD001-header.md(1 hunks)tools/include/markdown/ZFS001-footer.md(1 hunks)tools/include/markdown/ZFS001-header.md(1 hunks)tools/json/config.system.json(38 hunks)
✅ Files skipped from review due to trivial changes (7)
- tools/include/markdown/STABLE-header.md
- tools/include/markdown/STO001-header.md
- tools/include/markdown/UPD001-header.md
- tools/include/markdown/ZFS001-footer.md
- tools/include/markdown/BOOT01-header.md
- tools/include/markdown/ROO001-header.md
- tools/include/markdown/ZFS001-header.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/include/markdown/ACC001-header.md
- tools/include/markdown/NETFS01-header.md
- tools/include/markdown/HEAD01-header.md
- tools/include/markdown/SHELL1-header.md
- tools/json/config.system.json
🧰 Additional context used
🪛 Ruff (0.8.2)
tools/config-markdown.py
92-92: Loop control variable prefix not used within loop body
(B007)
109-109: Local variable cmd is assigned to but never used
Remove assignment to unused variable cmd
(F841)
137-137: Undefined name create_markdown_technical
(F821)
143-143: Local variable sub_anchors is assigned to but never used
Remove assignment to unused variable sub_anchors
(F841)
144-144: Undefined name create_markdown_technical
(F821)
🪛 LanguageTool
tools/include/markdown/DTO001-header.md
[grammar] ~1-~1: In the grammatical structure ‘pronoun + be/get + used + to + verb’, the verb ‘used’ is used with the gerund.
Context: ...without rebuilding the kernel. They are used to enable or configure specific hardware features...
(ADMIT_ENJOY_VB)
tools/include/markdown/KER001-header.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 355 characters long)
Context: ...ger "Kernel changes carry inherent risks!" - A mismatched or incompatible ke...
(EN_EXCESSIVE_EXCLAMATION)
tools/include/markdown/ROLLIN-header.md
[uncategorized] ~1-~1: Possible missing comma found.
Context: ...fixes, and hardware support improvements but may introduce instability or regression...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (2)
tools/include/markdown/DTO001-header.md (1)
1-1: LGTM! Clear and concise explanation of Device Tree OverlaysThe description effectively explains what Device Tree Overlays are, how they function, and their benefits for single-board computers in a concise manner.
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: In the grammatical structure ‘pronoun + be/get + used + to + verb’, the verb ‘used’ is used with the gerund.
Context: ...without rebuilding the kernel. They are used to enable or configure specific hardware features...(ADMIT_ENJOY_VB)
tools/include/markdown/KER001-header.md (1)
1-23: LGTM! Well-structured documentation with clear warnings and guidanceThe content provides comprehensive guidance on kernel switching risks, precautions, and recovery steps. The formatting with markdown admonitions, emphasis, and lists enhances readability and clearly communicates the potential dangers and necessary precautions.
🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 355 characters long)
Context: ...ger "Kernel changes carry inherent risks!" - A mismatched or incompatible ke...(EN_EXCESSIVE_EXCLAMATION)
igorpecovnik
force-pushed
the
improve_docs_gen
branch
4 times, most recently
from
a5f50cb to
2e98291
Compare
igorpecovnik
removed
Needs review
igorpecovnik
force-pushed
the
improve_docs_gen
branch
from
2e98291 to
9ed41c9
Compare
Description
Closing #330
Testing Procedure
It does expected output: #330
Checklist