Re: [PATCH] add-patch: response to invalid option

[phillip.wood123@gmail.com]

Hi Rubén

On 16/04/2024 20:24, Rubén Justo wrote:
> On Tue, Apr 16, 2024 at 10:41:32AM +0100, phillip.wood123@gmail.com wrote:
>> On 15/04/2024 20:00, Rubén Justo wrote:
>>>
>>> +		} else {
>>> +			err(s, _("Unknown option '%s' (use '?' for help)"),
>>
>> As this is an interactive program I think "Unknown key" would be clearer.
> 
> Maybe you have "interactive.singleKey" set to "true"?
> 
> Perhaps "key" refers more to the key pressed by the user.  My impression
> is that "option" have a clearer and wider audience.

I tend to associate "option" with a command-line argument, not 
interactive input to a program.

>> Something like this (which also adds coverage for '?' and 'p')
> 
> I know we don't have a test for the help.  I noticed this in my other
> series.  And I decided not to include a test for it then.  Maybe in this
> series it makes sense.
> 
> However, I would prefer to keep a test only on the new error message.

Why? This commit makes three changes
  - it stops printing the help when the user presses an unknown key
  - it starts treating '?' differently to an unknown key
  - it starts printing an error message when the user presses an unknown
    key

The test you are proposing only tests the last of these changes. We 
should be aiming to write tests that (a) verify all of the changes 
introduced by a commit (b) are likely to detect regressions to those 
changes (c) are reasonably efficient, for example if it is possible to 
test more than one key with a single "add -p" process we should do so. 
As this is an interactive program I have a strong preference for testing 
what the user sees printed to their screen, not just what happens to 
come out on stderr.

Thanks

Phillip