Refactor printConfig to display aligned types and enum options#945
Refactor printConfig to display aligned types and enum options#945Samith-NM wants to merge 1 commit into
Conversation
|
|
📝 WalkthroughWalkthroughThe printConfig function in meshtastic/main.py was modified to enrich per-field output with derived metadata including protobuf type labels, human-readable descriptions, boolean/options hints, and enum value listings, replacing the prior simple field-name listing with sorted, enriched output lines. ChangesprintConfig Output Enrichment
Estimated code review effort: 2 (Simple) | ~10 minutes Related PRs: None identified. Suggested labels: enhancement Suggested reviewers: None identified. 🐰 A rabbit taps keys with delicate care, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 (2)
meshtastic/__main__.py (2)
1222-1222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent capitalization: "options" vs "Options".
Boolean fields print
options: True/False(lowercase) while enum fields printOptions: ...(uppercase), creating an inconsistent CLI output style.✏️ Suggested fix
- opts = " || options: True/False" if f_type == "bool" else "" + opts = " || Options: True/False" if f_type == "bool" else ""Also applies to: 1224-1224
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshtastic/__main__.py` at line 1222, The CLI help text formatting is inconsistent between boolean and enum fields because the boolean branch in the field-description logic uses lowercase “options” while the enum branch uses uppercase “Options”. Update the string built in the relevant help/output path in meshtastic.__main__ so both branches use the same capitalization, and make sure the formatting stays consistent wherever these field types are rendered.
1211-1211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid reassigning the
configparameter.
config(the function parameter holding the outer message) is overwritten each iteration with a field descriptor fromfields_by_name. It still works here sinceobjDescwas already captured before the loop, but shadowing a parameter with an unrelated type is confusing and risks bugs if the function is extended later to reference the originalconfigafter this point.♻️ Suggested rename
for config_section in objDesc.fields: if config_section.name != "version": - config = objDesc.fields_by_name.get(config_section.name) + field_desc = objDesc.fields_by_name.get(config_section.name) print(f"{config_section.name}:") names = [] - for field in config.message_type.fields: + for field in field_desc.message_type.fields:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshtastic/__main__.py` at line 1211, The `config` parameter in the function containing `objDesc.fields_by_name.get(config_section.name)` is being reassigned to a field descriptor, which shadows the original outer message object and is confusing. Rename the local variable to something like `field` or `config_field` and update the subsequent references in that loop so the original `config` value remains intact for future use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meshtastic/__main__.py`:
- Line 1204: The printConfig type labeling is incomplete because type_map in
printConfig does not cover protobuf float fields, leaving config entries like
frequency_offset as unknown. Update the type resolution in printConfig by adding
the float cpp_type to type_map or deriving the label from field.cpp_type so all
supported config field types are printed consistently. Keep the change localized
to the type_map/field type handling logic in printConfig.
---
Nitpick comments:
In `@meshtastic/__main__.py`:
- Line 1222: The CLI help text formatting is inconsistent between boolean and
enum fields because the boolean branch in the field-description logic uses
lowercase “options” while the enum branch uses uppercase “Options”. Update the
string built in the relevant help/output path in meshtastic.__main__ so both
branches use the same capitalization, and make sure the formatting stays
consistent wherever these field types are rendered.
- Line 1211: The `config` parameter in the function containing
`objDesc.fields_by_name.get(config_section.name)` is being reassigned to a field
descriptor, which shadows the original outer message object and is confusing.
Rename the local variable to something like `field` or `config_field` and update
the subsequent references in that loop so the original `config` value remains
intact for future use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 60b4dc3e-2aa1-4e67-996a-9cffe80ac93b
📒 Files selected for processing (1)
meshtastic/__main__.py
2 participants
modified the printConfig function to make more user friendly interaction in CLI as per the issue #393
Summary by CodeRabbit