Generate shell completion scripts #36
Conversation
WalkthroughThe pull request introduces shell completion script generation for a command-line application. It adds the Changes
Sequence DiagramsequenceDiagram
participant Build as Build Script
participant CLI as CLI Args
participant Shell as Shell Environment
Build->>CLI: Define Command Structure
Build->>Build: Determine Output Directory
Build->>Shell: Generate Completion Scripts
Shell-->>Build: Save Completion Scripts
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
build.rs (1)
14-16: Add progress feedback during generationConsider adding build script output to inform about the completion script generation progress.
for &shell in Shell::value_variants() { + println!("cargo:warning=Generating {} completion script...", shell); generate_to(shell, &mut cmd, "pestfmt", &out_dir).unwrap(); }src/cli.rs (1)
7-9: Improve documentation for thefilefieldThe documentation should clarify that "." means the current directory and that multiple files/paths can be provided.
- /// The file or path to format + /// One or more files or directories to format + /// If a directory is specified, all .pest files in it will be formatted recursively + /// Defaults to the current directory (".")Cargo.toml (1)
19-19: Consider using workspace dependenciesThe clap and clap_complete dependencies are duplicated in both
[dependencies]and[build-dependencies]. Consider using workspace dependencies to maintain consistent versions.[workspace.dependencies] clap = { version = "4.4.15", features = ["derive"] } clap_complete = "4.5.42" [dependencies] -clap = { version = "4.4.15", features = ["derive"] } -clap_complete = "4.5.42" +clap = { workspace = true } +clap_complete = { workspace = true } [build-dependencies] -clap = { version = "4.4.15", features = ["derive"] } -clap_complete = "4.5.42" +clap = { workspace = true } +clap_complete = { workspace = true }Also applies to: 27-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Cargo.toml(1 hunks)build.rs(1 hunks)src/cli.rs(1 hunks)src/main.rs(1 hunks)
🔇 Additional comments (1)
src/main.rs (1)
13-16: 🛠️ Refactor suggestionImprove error handling for completion generation
The completion generation should handle potential errors from print_completions.
if let Some(generator) = cli.gen_completion { let mut cmd = cli::Args::command(); - cli::print_completions(generator, &mut cmd); + cli::print_completions(generator, &mut cmd) + .map_err(|e| format!("Failed to generate completion script: {}", e))?; } else if cli.stdin {Likely invalid or redundant comment.
src/cli.rs
Outdated
| pub fn print_completions<G: Generator>(gen: G, cmd: &mut Command) { | ||
| generate(gen, cmd, cmd.get_name().to_string(), &mut std::io::stdout()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling in print_completions
The function writes directly to stdout without handling potential I/O errors.
-pub fn print_completions<G: Generator>(gen: G, cmd: &mut Command) {
+pub fn print_completions<G: Generator>(gen: G, cmd: &mut Command) -> std::io::Result<()> {
- generate(gen, cmd, cmd.get_name().to_string(), &mut std::io::stdout());
+ generate(gen, cmd, cmd.get_name().to_string(), &mut std::io::stdout())?;
+ Ok(())
}📝 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.
| pub fn print_completions<G: Generator>(gen: G, cmd: &mut Command) { | |
| generate(gen, cmd, cmd.get_name().to_string(), &mut std::io::stdout()); | |
| } | |
| pub fn print_completions<G: Generator>(gen: G, cmd: &mut Command) -> std::io::Result<()> { | |
| generate(gen, cmd, cmd.get_name().to_string(), &mut std::io::stdout())?; | |
| Ok(()) | |
| } |
romch007
force-pushed
the
shell-completions
branch
from
dc39619 to
20b307c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli.rs (1)
4-17: Add missing documentation for thestdinfield.While the
fileandgen_completionfields have documentation comments, thestdinfield lacks documentation. Consider adding a doc comment to maintain consistency and improve code documentation.#[arg(default_value = ".")] pub file: Vec<String>, + /// Read from standard input instead of files #[clap(long, short, default_value = "false")] pub stdin: bool,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Cargo.toml(1 hunks)build.rs(1 hunks)src/cli.rs(1 hunks)src/main.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- build.rs
- Cargo.toml
🔇 Additional comments (3)
src/cli.rs (2)
1-2: LGTM! Appropriate imports for CLI and completion generation.The imports correctly bring in the necessary components from
clapandclap_completefor argument parsing and shell completion generation.
19-23: Consider error handling in print_completionsThe function writes directly to stdout without handling potential I/O errors.
Consider making the binary name configurable.
The binary name "pestfmt" is hardcoded in the completion generation. Consider making it configurable or deriving it from the package metadata to ensure it stays in sync with the actual binary name.
-pub fn print_completions<G: Generator>(gen: G) { +pub fn print_completions<G: Generator>(gen: G, binary_name: &str) -> std::io::Result<()> { let mut cmd = Args::command(); - generate(gen, &mut cmd, "pestfmt", &mut std::io::stdout()); + generate(gen, &mut cmd, binary_name, &mut std::io::stdout())?; + Ok(()) }src/main.rs (1)
1-2: LGTM! Clean integration of completion generation.The new completion generation feature is well-integrated into the existing codebase without disrupting the current functionality. The command-line argument parsing changes are minimal and maintain backward compatibility.
Also applies to: 11-15
| let cli = cli::Args::parse(); | ||
|
|
||
| if cli.stdin { | ||
| if let Some(generator) = cli.gen_completion { | ||
| cli::print_completions(generator); | ||
| } else if cli.stdin { |
There was a problem hiding this comment.
Ensure consistent error handling for completion generation.
The completion generation branch doesn't propagate potential I/O errors, unlike other branches that use the ? operator. This could lead to silent failures when generating completions.
let cli = cli::Args::parse();
if let Some(generator) = cli.gen_completion {
- cli::print_completions(generator);
+ cli::print_completions(generator)?;
} else if cli.stdin {📝 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.
| let cli = cli::Args::parse(); | |
| if cli.stdin { | |
| if let Some(generator) = cli.gen_completion { | |
| cli::print_completions(generator); | |
| } else if cli.stdin { | |
| let cli = cli::Args::parse(); | |
| if let Some(generator) = cli.gen_completion { | |
| cli::print_completions(generator)?; | |
| } else if cli.stdin { |
|
I think this feature is unnecessary. The pestfmt function is quite simple. There is no need to introduce this feature to increase maintenance complexity. We should not add it just for the sake of adding it. |
|
@romch007 thank you for your contribution. As @huacnlee noted, the tradeoff of extra code maintenance vs utility it gives isn't likely worth adding it at this time. |
This PR adds shell completion script generation using
clap_complete, both at build time and runtime. There is now a new command line option:The completion scripts will also be generated in
target/debug/build/pest_fmt-xxxxxxxxxxxxxxx/out/Summary by CodeRabbit
New Features
Chores