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

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Apr 17, 2024, 09:37 UTC
Message-ID: <1d0e98cb-78a4-40d0-9bfe-390a3a30aad8@gmail.com>
In-Reply-To: <20685fa0-815f-4cdf-95e0-7206588552b5@gmail.com>

Hi Rubén

On 16/04/2024 20:24, Rubén Justo wrote:

Show 12 quoted lines

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

Show 7 quoted lines

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