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

Thomas Lamprecht t.lamprecht at proxmox.com
Wed Apr 24 21:03:42 CEST 2024


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.

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.

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)
+    /// 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.
-- 
2.39.2









More information about the pbs-devel mailing list