[pbs-devel] [PATCH proxmox v5 1/2] router: cli: added `ask_for_confirmation` helper

[Gabriel Goller]

On Wed Apr 24, 2024 at 9:03 PM CEST, Thomas Lamprecht wrote:
> Am 18/04/2024 um 13:49 schrieb Gabriel Goller:
> > Added `ask_for_confirmation` helper that outputs a prompt and
> > lets the user confirm or deny it.
> > Implemented to close #4763.
> > 
> > Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
> > ---
> >  proxmox-router/Cargo.toml     |  1 +
> >  proxmox-router/src/cli/mod.rs | 46 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 46 insertions(+), 1 deletion(-)
> > 
> > diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml
> > index dcd71a4..0b9d361 100644
> > --- a/proxmox-router/Cargo.toml
> > +++ b/proxmox-router/Cargo.toml
> > @@ -19,6 +19,7 @@ percent-encoding.workspace = true
> >  serde_json.workspace = true
> >  serde.workspace = true
> >  unicode-width ="0.1.8"
> > +regex.workspace = true
> >  
> >  # cli:
> >  tokio = { workspace = true, features = [], optional = true }
> > diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
> > index 7df94ad..7f87284 100644
> > --- a/proxmox-router/src/cli/mod.rs
> > +++ b/proxmox-router/src/cli/mod.rs
> > @@ -12,7 +12,10 @@
> >  //! - Ability to create interactive commands (using ``rustyline``)
> >  //! - Supports complex/nested commands
> >  
> > -use std::collections::HashMap;
> > +use std::{
> > +    collections::HashMap,
> > +    io::{self, Write},
> > +};
> >  
> >  use crate::ApiMethod;
> >  
> > @@ -61,6 +64,47 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
> >      .init();
> >  }
> >  
> > +pub enum DefaultAnswer {
>
> The enumeration is not really tied to being a _default_ answer, you just use
> it that way now in this patch, I'd rather use just Answer and then also use that
> as return value.

I agree.

>
> See for a replacement proposal at the end.
>
> > +    Yes,
> > +    No,
> > +}
> > +
> > +/// Prints a prompt to ask for confirmation
> > +pub fn ask_for_confirmation(query: String, default: DefaultAnswer) -> Result<bool, io::Error> {
> > +    let yesnoprompt: (char, char) = match default {
> > +        DefaultAnswer::Yes => ('Y', 'n'),
> > +        DefaultAnswer::No => ('y', 'N'),
> > +    };
> > +    print!("{query} [{}/{}]: ", yesnoprompt.0, yesnoprompt.1);
> > +
> > +    io::stdout().flush()?;
> > +    let stdin = io::stdin();
> > +    let mut line = String::new();
> > +    stdin.read_line(&mut line)?;
> > +
> > +    use regex::Regex;
> > +    match default {
> > +        DefaultAnswer::Yes => {
> > +            // unwrap is okay, because this regex is correct
> > +            let no_regex: Regex = Regex::new("^[nN]$").unwrap();
> > +            if no_regex.is_match(line.trim()) {
> > +                Ok(false)
> > +            } else {
> > +                Ok(true)
>
> So, a caller passes DefaultAnswer::Yes, the user enter the full word "no", your logic
> then makes it a Yes like it would for all other unrecognized input...  I do not like
> such dangerous interfaces, so NACK.

I'd have to disagree, I don't think this is a dangerous interface...
If, e.g., we use DefaultAnswer::No, there is no difference between 'no',
'bogus', 'N', or '<enter>' IMO. So why should one return an error and
the other one false?

This was just my thought when implementing this, although ultimately,
it doesn't matter because we only check for success and ignore errors 
and false returns.
FWIW I think your implementation is way prettier, especially the Answer
enum being input and output :)

Should I apply the diff and submit a new version or can this be applied
immediately?

>
> The default must only be taken if the user did not make any input, but just pressed
> enter.
>
> > +            }
> > +        }
> > +        DefaultAnswer::No => {
> > +            // unwrap is okay, because this regex is coorrect
> > +            let yes_regex: Regex = Regex::new("^[yY]$").unwrap();
> > +            if yes_regex.is_match(line.trim()) {
> > +                Ok(true)
> > +            } else {
> > +                Ok(false)
>
> same here
>
> > +            }
> > +        }
>
> I'd rather prefer something like the following interface, it's mostly boilerplate
> and docs, but it should allow some safer usage. Could also do s/Confirmation/Confirm/
> but no hard feelings there.
>
> The code from patch 2/2 could look the roughly like:
>
> let destroy_group_answer = Confirmation:query_with_default(
>     format!("Delete group \"{backup_group}\" with {snapshots} snapshot(s)?"),
>     Confirmation:No,
> )?;
>
> if destroy_group_answer.is_yes() {
>     // ....
> }
>
>
> ----8<----
> From 6287a4dd3f092e2413356798a41e3a7423536f98 Mon Sep 17 00:00:00 2001
> From: Thomas Lamprecht <t.lamprecht at proxmox.com>
> Date: Wed, 24 Apr 2024 20:55:43 +0200
> Subject: [PATCH] router: cli: add yes/no confirmation enum with prompt query
>  helper
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
>  proxmox-router/src/cli/mod.rs | 115 +++++++++++++++++++++++++++++++++-
>  1 file changed, 114 insertions(+), 1 deletion(-)
>
> diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
> index 7df94ad9..7e561b33 100644
> --- a/proxmox-router/src/cli/mod.rs
> +++ b/proxmox-router/src/cli/mod.rs
> @@ -12,7 +12,10 @@
>  //! - Ability to create interactive commands (using ``rustyline``)
>  //! - Supports complex/nested commands
>  
> -use std::collections::HashMap;
> +use std::{
> +    collections::HashMap,
> +    io::{self, Write},
> +};
>  
>  use crate::ApiMethod;
>  
> @@ -61,6 +64,116 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
>      .init();
>  }
>  
> +#[derive(PartialEq, Debug)]
> +/// Use for simple yes or no questions, where booleans can be confusing, especially if there's a
> +/// default response to consider. The implementation provides query helper for the CLI.
> +pub enum Confirmation {
> +    Yes,
> +    No,
> +}
> +
> +impl Confirmation {
> +    /// get the formatted choice for the query prompt, with self being the highlighted (default)

Nit: 'get' -> 'Get'

> +    /// one displayed as upper case.
> +    pub fn default_choice_str(&self) -> &'static str {
> +        match self {
> +            Self::Yes => "Y/n",
> +            Self::No => "y/N",
> +        }
> +    }
> +
> +    /// Returns true if the answer is Yes
> +    pub fn is_yes(&self) -> bool {
> +        *self == Self::Yes
> +    }
> +
> +    /// Returns true if the answer is No
> +    pub fn is_no(&self) -> bool {
> +        *self == Self::No
> +    }
> +
> +    /// Parse an input string reference as yes or no confirmation.
> +    ///
> +    /// The input string is checked verbatim if it is exactly one of the single chars 'y', 'Y',
> +    /// 'n', or 'N'. You must trim the string before calling, if needed, or use one of the query
> +    /// helper functions.
> +    ///
> +    /// ```
> +    /// use proxmox_router::cli::Confirmation;
> +    ///
> +    /// let answer = Confirmation::from_str("y");
> +    /// assert!(answer.expect("valid").is_yes());
> +    ///
> +    /// let answer = Confirmation::from_str("N");
> +    /// assert!(answer.expect("valid").is_no());
> +    ///
> +    /// let answer = Confirmation::from_str("bogus");
> +    /// assert!(answer.is_err());
> +    /// ```
> +    pub fn from_str(input: &str) -> Result<Self, io::Error> {
> +        match input.trim() {
> +            "y" | "Y" => Ok(Self::Yes),
> +            "n" | "N" => Ok(Self::No),
> +            _ => Err(io::Error::new(
> +                io::ErrorKind::InvalidInput,
> +                "invalid input, enter one of y, Y, n, or N",
> +            )),
> +        }
> +    }
> +
> +    /// Parse a input string reference as yes or no confirmation, allowing a fallback default
> +    /// answer if the user enters an empty choice.
> +    ///
> +    /// The input string is checked verbatim if it is exactly one of the single chars 'y', 'Y',
> +    /// 'n', or 'N'. The empty string maps to the default. You must trim the string before calling,
> +    /// if needed, or use one of the query helper functions.
> +    ///
> +    /// ```
> +    /// use proxmox_router::cli::Confirmation;
> +    ///
> +    /// let answer = Confirmation::from_str_with_default("", Confirmation::No);
> +    /// assert!(answer.expect("valid").is_no());
> +    ///
> +    /// let answer = Confirmation::from_str_with_default("n", Confirmation::Yes);
> +    /// assert!(answer.expect("valid").is_no());
> +    ///
> +    /// let answer = Confirmation::from_str_with_default("yes", Confirmation::Yes);
> +    /// assert!(answer.is_err()); // full-word answer not allowed for now.
> +    /// ```
> +    pub fn from_str_with_default(input: &str, default: Self) -> Result<Self, io::Error> {
> +        match input.trim() {
> +            "y" | "Y" => Ok(Self::Yes),
> +            "n" | "N" => Ok(Self::No),
> +            "" => Ok(default),
> +            _ => Err(io::Error::from(io::ErrorKind::InvalidInput)),
> +        }
> +    }
> +
> +    /// Print a query prompt with available yes no choices and returns the String the user enters.
> +    fn read_line(query: &str, choices: &str) -> Result<String, io::Error> {
> +        print!("{query} [{choices}]: ");
> +
> +        io::stdout().flush()?;
> +        let stdin = io::stdin();
> +        let mut line = String::new();
> +        stdin.read_line(&mut line)?;
> +        Ok(line)
> +    }
> +
> +    /// Print a query prompt and parse the white-space trimmed answer using from_str.
> +    pub fn query(query: &str) -> Result<Self, io::Error> {
> +        let line = Self::read_line(query, "y/n")?;
> +        Confirmation::from_str(line.trim())
> +    }
> +
> +    /// Print a query prompt and parse the answer using from_str_with_default, falling back to the
> +    /// default_answer if the user provided an empty string.
> +    pub fn query_with_default(query: &str, default_answer: Self) -> Result<Self, io::Error> {
> +        let line = Self::read_line(query, default_answer.default_choice_str())?;
> +        Confirmation::from_str_with_default(line.trim(), default_answer)
> +    }
> +}
> +
>  /// Define a simple CLI command.
>  pub struct CliCommand {
>      /// The Schema definition.