ci: check OPTIONS.rst freshness#6940
Conversation
WalkthroughThis PR adds a Python-side documentation check and integrates it into the repository verification flow. The generator script python/helper-scripts/make-options-rst.py was refactored to accept --output and use pathlib; python/Makefile gained a doc_check target that diffs generated OPTIONS.rst against docs/OPTIONS.rst; the top-level Makefile now invokes that check via new python_doc and python_doc_check targets and adds python_doc_check to gen_check. The generated python/docs/OPTIONS.rst was updated with two new command entries and a CLI help docstring was added to evolu.index_management. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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 `@python/docs/OPTIONS.rst`:
- Line 316: The index_management click command in
python/src/trezorlib/cli/evolu.py is missing a docstring so its help text is
blank; add a descriptive single-line (or short multi-line) docstring immediately
below the def index_management(...) declaration that explains what the command
does, documents the rotation_index option (e.g., its purpose and that it’s
optional), and briefly states the return value (rotation index as string) so
Click will include this description in generated help and docs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 139032fe-0e4f-4a81-adc8-22cac0004d2d
📒 Files selected for processing (2)
Makefilepython/docs/OPTIONS.rst
|
Thanks @MukundaKatta for your contribution. Please, make the following changes in order for the PR to be accepted:
|
|
Addressed the review feedback in d8ea921:
Validated locally:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/helper-scripts/make-options-rst.py (1)
41-45: ⚡ Quick winConsider validating that the delimiter was found.
The loop reads
OPTIONS_RSTuntil it findsDELIMITER_STR, but if the delimiter is never found, the script silently reads the entire file intolead_inand continues. This could lead to unexpected output if the file format changes or the delimiter is accidentally removed.🛡️ Proposed validation check
lead_in: list[str] = [] +found_delimiter = False with OPTIONS_RST.open() as options_rst: for line in options_rst: lead_in.append(line) if DELIMITER_STR in line: + found_delimiter = True break +if not found_delimiter: + raise ValueError(f"Delimiter '{DELIMITER_STR}' not found in {OPTIONS_RST}") + output: TextIO output = args.output.open("w")🤖 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 `@python/helper-scripts/make-options-rst.py` around lines 41 - 45, The loop that reads OPTIONS_RST into lead_in stops only when DELIMITER_STR is seen, but currently doesn't detect the case where the delimiter is missing; update the reader logic that iterates over OPTIONS_RST so after the loop you check whether any line contained DELIMITER_STR (e.g., track a boolean found_delimiter or inspect the last appended lines for DELIMITER_STR) and if not, raise an exception or exit with an error message via logging that mentions OPTIONS_RST and DELIMITER_STR so the failure is explicit; reference the existing variables/options: OPTIONS_RST, DELIMITER_STR, and lead_in when implementing the validation and error handling.
🤖 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 `@Makefile`:
- Around line 172-176: Add a .PHONY declaration listing the non-file targets so
make won't treat them as files: declare .PHONY for python_doc, python_doc_check
and also include the existing gen and gen_check targets (and any other similar
meta-targets) by adding a .PHONY line that references those target names; update
the Makefile near existing target definitions to include ".PHONY: python_doc
python_doc_check gen gen_check" so these commands are always executed regardless
of filesystem state.
---
Nitpick comments:
In `@python/helper-scripts/make-options-rst.py`:
- Around line 41-45: The loop that reads OPTIONS_RST into lead_in stops only
when DELIMITER_STR is seen, but currently doesn't detect the case where the
delimiter is missing; update the reader logic that iterates over OPTIONS_RST so
after the loop you check whether any line contained DELIMITER_STR (e.g., track a
boolean found_delimiter or inspect the last appended lines for DELIMITER_STR)
and if not, raise an exception or exit with an error message via logging that
mentions OPTIONS_RST and DELIMITER_STR so the failure is explicit; reference the
existing variables/options: OPTIONS_RST, DELIMITER_STR, and lead_in when
implementing the validation and error handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e10115f-d468-4ca6-8f46-e14e89925647
📒 Files selected for processing (5)
Makefilepython/Makefilepython/docs/OPTIONS.rstpython/helper-scripts/make-options-rst.pypython/src/trezorlib/cli/evolu.py
✅ Files skipped from review due to trivial changes (2)
- python/src/trezorlib/cli/evolu.py
- python/docs/OPTIONS.rst
| python_doc: ## generate trezorctl OPTIONS.rst | ||
| make -C python doc | ||
|
|
||
| python_doc_check: ## check that trezorctl OPTIONS.rst is up to date | ||
| make -C python doc_check |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for .PHONY declarations of gen targets
rg -n '\.PHONY.*\b(gen|gen_check|python_doc|python_doc_check)\b' MakefileRepository: trezor/trezor-firmware
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check all .PHONY declarations in the Makefile
rg -n '\.PHONY' Makefile -A 1Repository: trezor/trezor-firmware
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if gen and gen_check targets exist
rg -n '^(gen|gen_check|python_doc|python_doc_check):' Makefile -A 2Repository: trezor/trezor-firmware
Length of output: 750
🏁 Script executed:
#!/bin/bash
# Check if there are any files named gen, gen_check, python_doc, or python_doc_check
fd -e gen -e gen_check -e python_doc -e python_doc_check . 2>/dev/null | head -20Repository: trezor/trezor-firmware
Length of output: 48
Add .PHONY declarations for targets.
The Makefile currently has no .PHONY declarations. The new python_doc and python_doc_check targets, as well as the existing gen and gen_check targets, are all non-file-producing targets and should be declared as .PHONY to prevent potential conflicts and follow make best practices.
📝 Proposed PHONY declarations
+.PHONY: python_doc python_doc_check gen gen_check
+
python_doc: ## generate trezorctl OPTIONS.rst
make -C python doc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| python_doc: ## generate trezorctl OPTIONS.rst | |
| make -C python doc | |
| python_doc_check: ## check that trezorctl OPTIONS.rst is up to date | |
| make -C python doc_check | |
| .PHONY: python_doc python_doc_check gen gen_check | |
| python_doc: ## generate trezorctl OPTIONS.rst | |
| make -C python doc | |
| python_doc_check: ## check that trezorctl OPTIONS.rst is up to date | |
| make -C python doc_check |
🤖 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 `@Makefile` around lines 172 - 176, Add a .PHONY declaration listing the
non-file targets so make won't treat them as files: declare .PHONY for
python_doc, python_doc_check and also include the existing gen and gen_check
targets (and any other similar meta-targets) by adding a .PHONY line that
references those target names; update the Makefile near existing target
definitions to include ".PHONY: python_doc python_doc_check gen gen_check" so
these commands are always executed regardless of filesystem state.
2 participants
Description
options_checktarget that regeneratespython/docs/OPTIONS.rstand fails if it changesgen_check, so the existing Prebuild checks workflow catches stale CLI docsOPTIONS.rstbaseline with the same two generated entries as chore(docs): updateOPTIONS.rst#6937, so this check can start green even if this PR merges firstFixes #6938
Testing
git submodule update --init --recursive vendor/ts-tvluv run make options_checkgit diff --cached --check