diff options
| author | Karl Ding <karlding@users.noreply.github.com> | 2025-10-20 13:07:41 -0700 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2025-10-20 13:07:41 -0700 |
| commit | aa9e5ad1d55cf6b82127f080be0f30a0c2c9f7e7 (patch) | |
| tree | 90937fc94994d595c2991f84e1e4cc5213da69ae /crates | |
| parent | feat: Interactive Inspector (#2319) (diff) | |
| download | atuin-aa9e5ad1d55cf6b82127f080be0f30a0c2c9f7e7.zip | |
fix: stats ngram window size cli parsing (#2946)
Fix panic when running "atuin stats" with an empty window size:
$ RUST_BACKTRACE=1 atuin stats year -n0
thread 'main' panicked at crates/atuin-history/src/stats.rs:278:14:
window size must be non-zero
stack backtrace:
0: __rustc::rust_begin_unwind
1: core::panicking::panic_fmt
2: core::option::expect_failed
3: atuin_history::stats::compute
4: atuin::command::client::Cmd::run_inner::{{closure}}
5: tokio::runtime::context::scoped::Scoped<T>::set
6: tokio::runtime::scheduler::current_thread::CoreGuard::block_on
7: tokio::runtime::context::runtime::enter_runtime
8: atuin::command::AtuinCmd::run
9: atuin::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a
verbose backtrace.
The ngram window size had a default value of 1 (indicating that perhaps
this case was considered), but did not validate the parsed value when a
non-default value was given.
Such validation can either occur:
1. during clap argument parsing
2. after clap parses arguments, but before computing statistics
If we were to do this after clap finishes parsing arguments, this would
be a runtime check in run(), but it seems better to return an error as
soon as possible.
It's possible to do this without a custom parsing function via
clap::value_parser! and specifying a range:
clap::value_parser!(u64).range(1..)
However, this does not appear to support custom error messages so we
would be left with the default message specifying an invalid range.
Therefore, add a custom parser function and return an error when the
value cannot be parsed as a valid ngram value.
<!-- Thank you for making a PR! Bug fixes are always welcome, but if
you're adding a new feature or changing an existing one, we'd really
appreciate if you open an issue, post on the forum, or drop in on
Discord -->
## Checks
- [x] I am happy for maintainers to push small adjustments to this PR,
to speed up the review cycle
- [x] I have checked that there are no existing pull requests for the
same thing
Diffstat (limited to 'crates')
| -rw-r--r-- | crates/atuin/src/command/client/stats.rs | 14 |
1 files changed, 13 insertions, 1 deletions
diff --git a/crates/atuin/src/command/client/stats.rs b/crates/atuin/src/command/client/stats.rs index daffdd84..3ee60633 100644 --- a/crates/atuin/src/command/client/stats.rs +++ b/crates/atuin/src/command/client/stats.rs @@ -11,6 +11,18 @@ use atuin_client::{ use atuin_history::stats::{compute, pretty_print}; +fn parse_ngram_size(s: &str) -> Result<usize, String> { + let value = s + .parse::<usize>() + .map_err(|_| format!("'{s}' is not a valid window size"))?; + + if value == 0 { + return Err("ngram window size must be at least 1".to_string()); + } + + Ok(value) +} + #[derive(Parser, Debug)] #[command(infer_subcommands = true)] pub struct Cmd { @@ -22,7 +34,7 @@ pub struct Cmd { count: usize, /// The number of consecutive commands to consider - #[arg(long, short, default_value = "1")] + #[arg(long, short, default_value = "1", value_parser = parse_ngram_size)] ngram_size: usize, } |
