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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.


> 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.


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