-
Notifications
You must be signed in to change notification settings - Fork 23
feat: added docgen for doc autogeneration #526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. This is nice! I like the use of clap
built-in functions to extract the CLI help text. Some comments:
-
cargo fmt
is failing. You can runcargo +nightly fmt --all
to fix this -
The goal of the docgen is to have it integrated in the CI, so that whenever a doc is modified, it must be updated or fails the CI.
Therefore, could you also modify this script and integrate in the CI? The script is copied from Lighthouse where we generate the CLI help text using --help
, but in this case, we can do so with anchor docgen
, and then compare with the existing docs. In the future, when devs are updating the doc, they just have to run the script and the doc can be updated.
Thanks!
anchor/src/docs.rs
Outdated
let default = if arg.get_default_values().is_empty() { | ||
"None".to_string() | ||
} else { | ||
let values: Vec<String> = arg | ||
.get_default_values() | ||
.iter() | ||
.map(|v| v.to_string_lossy().to_string()) | ||
.collect(); | ||
format!("`{}`", values.join(", ")) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about #2 in your original post on default values, I can see that the docgen
can capture the default values if there is default_value
in the clap, example:
anchor/anchor/client/src/cli.rs
Line 217 in a79943b
default_value = "0.0.0.0", |
There are some flags with default_value_if
that are not captured. Not sure if we can have another else if
arm to check for these flags? The get_default_values
is returning this field: https://docs.rs/clap_builder/4.5.47/src/clap_builder/builder/arg.rs.html#4341
Although I don't see the method to "get default values if" but I wonder if we can do something similar like arg.default_vals_ifs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put the condition in long_help
?
@@ -24,6 +24,7 @@ http_api = { workspace = true } | |||
http_metrics = { workspace = true } | |||
hyper = { workspace = true } | |||
keygen = { workspace = true } | |||
keysplit = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add keysplit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. It would be nicer to have the docgen as a separate binary target of the top level crate IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi what would be better, a separate binary target for docgen or have a top level library crate that it then called into the main anchor binary as a CLI command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My recommendation: make docgen a separate workspace binary (e.g. tools/docgen) rather than an anchor client subcommand. This keeps the shipping CLI lean, avoids linking extra deps (e.g., keysplit) into the client crate.
Sketch:
• tools/docgen/Cargo.toml (bin = docgen)
• Depends on the crates that define the clap trees (e.g. anchor-client, keygen, keysplit) via path = "../anchor/client" etc.
• Each of those crates exposes a small pub fn cli() -> clap::Command (or impl CommandFactory) so docgen can introspect without booting the whole app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces an automated documentation generation system for CLI commands using clap's CommandFactory
trait. The system generates markdown documentation for the node, keygen, and keysplit commands by introspecting their CLI argument definitions.
Key changes:
- Added a new
docgen
subcommand that generates CLI documentation automatically - Refactored the Node CLI structure into separate grouped structs for better organization
- Updated documentation files with auto-generated content and version bumps
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
anchor/src/main.rs | Added new docgen subcommand to main CLI |
anchor/client/src/docs.rs | New documentation generator implementation using clap introspection |
anchor/client/src/cli.rs | Refactored Node struct into grouped option structs for better organization |
anchor/client/src/config.rs | Updated to use new nested CLI structure for configuration parsing |
docs/docs/pages/*.mdx | Auto-generated documentation files with updated content and version numbers |
docs/vocs.config.ts | Version bump from v0.2.0 to v0.3.1 |
Comments suppressed due to low confidence (1)
anchor/client/src/docs.rs:1
- The condition
id.contains(\"nodes\")
is checked twice in consecutive match arms. This creates unreachable code since line 285 will always match before line 286 for IDs containing "nodes".
use std::{env, fs};
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| `--keystore-path <PATH>` | Path to the validator keystore file | None | | ||
| `--password <VALUE>` | Password for the validator keystore | None | | ||
| `--owner <VALUE>` | EOA address that owns the validator | None | | ||
| `--output-path <PATH>` | Path for output | None | | ||
| `--operators <VALUE>` | Operators to split key among | None | | ||
| `--nonce <VALUE>` | Nonce for the owner address | None | | ||
| `--public-keys <VALUE>` | RSA public keys for the operators | None | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manual-specific options table duplicates all the shared options. This creates maintenance overhead as changes to shared options need to be updated in multiple places.
Copilot uses AI. Check for mistakes.
Hi guys, any suggestions on the 2 unsolved comments? |
Would using |
Issue Addressed
#524
Which issue # does this PR address?
Auto generation of docs from clap cli args
Additional Info
I thought implementing it directly via clap's
CommandFactory
is a much cleaner way to get all the arguments and generate docs out of it. This is my implementation of how I think it could be done.Tried to match the exact structure of the docs which were currently there. Current issues/points that could be discussed could be -
md
docs.clap
which is making it hard to get the default values.....examples
in md could be autogenerated. Haven't thought much about that yet.PS - The current docs in this branch are generated via the
docgen
.cc for reviews: @chong-he @dknopik