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

["Gabriel Goller" <g.goller@proxmox.com>]

On Thu Apr 25, 2024 at 11:42 AM CEST, Thomas Lamprecht wrote:
> Am 25/04/2024 um 10:52 schrieb 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:
> >>> +    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?
>
> I do think there's a big difference though. Because with "" the user entered
> the empty choice to get the default that was presented to them, with uppercase
> signalling what the default is, and for the others the user entered an unknown,
> invalid choice that one must not derive any action from.
>
> And while yes, as long as the default is a no-op it would be indeed fine to
> silently ignore any bogus input and relay the default answer, but this here
> is a general interface that cannot take such assumptions because it just
> cannot know possibly implications of that choice, the default choice is
> not bound to be the no-op after all.
>
> E.g., if you have an interface that allows for some changes and then could
> sync those to remotes (this is roughly how our CDN repo tooling works).
> It's not a real destructive action and most often syncing is what you want,
> so defaulting to yes is done. But sometimes one must not sync immediately
> due to batching changes or redoing them. Now, what's the better UI for
> following prompt + entry
>
> The proposal of the original patch:
>
> ```
> Sync? [Y/n]: no
> Ok, syncing...
> ```
>
>
> ```
> Sync? [Y/n]: no
> Abort – unexpected choice "no"! Use enter for default or use 'y' or 'n'.
> ```
>
> IMO the latter is much more resilient to human errors, e.g. a middle click
> pasting in some text that contains a newline, or a simple misunderstanding.
> And FWIW also more unlikely, but not unheard of, things like spilling
> drinks, or toddlers or cats walking over the keyboard – which for the
> "accept any invalid input as default answer" has a quite high chance to
> trigger something unexpected.
>
> In short, for confirmation the behavior should be clear and expected,
> accepting arbitrary data as valid choice isn't either; erroring out might
> be a nuisance for some, but is safe and provides clarity.

Ok, you convinced me :)

> > 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?
>
> My code was mostly an elaborate sketch, I could be improved a bit, i.e.,
> what I did not really thought through is using std::io::Error as Error type,
> at least for the "invalid input" case it might be maybe debatable,
> but using that now would allow users to avoid forcing the use of a more
> complex one, like anyhow, and can also be changed later.

The current output on error is: 

    Error: invalid input parameter

anyhow is already used in proxmox-router so I'd say we can just as well
use it. A simple bail! with the message you proposed should do the trick:

    Abort – unexpected choice "no"! Use enter for default or use 'y' or 'n'.




_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel