bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Daniel Mendler]

On 1/19/23 12:05, Robert Pluim wrote:
>>>>>> On Thu, 19 Jan 2023 11:39:34 +0100, Daniel Mendler 
>>>>>> <mail@daniel-mendler.de> said:
> 
>     Daniel> On 1/19/23 11:16, Robert Pluim wrote:
>     Daniel> Robert, speaking of `key-parse', I wonder why this function 
> accepts
>     Daniel> invalid key strings. Why don't we move the `key-valid-p' check to
>     Daniel> `key-parse'? This would alleviate the need for many additional
>     Daniel> `key-valid-p' checks across keymap.el. One could maybe even get 
> rid of
>     Daniel> the `keymap--check' helper.
>     >> 
>     >> Do you have an example of such an invalid string?
> 
>     Daniel> Just try something like this:
> 
>     Daniel> (key-valid-p "bug")
>     Daniel> (key-parse "bug")
> 
> Ah, the old "do we have to put spaces between keys" issue. Iʼm not
> sure how best to deal with that, Iʼll have to read through keymap.el
> some more. This:

My point is that it would be expected from `key-parse' that it is
equally strict as the other keymap functions, otherwise we miss bugs
where `key-parse' wasn't used properly. Furthermore we would avoid all
these `key-valid-p` and `keymap--check' calls, as I mentioned.

Of course `kbd' should stay as lax as it has always been.

>     >> I think itʼs too late to make such changes for emacs-29. Iʼm not even
>     >> sure that the minimal changes I proposed will be accepted :-)
> 
>     Daniel> I don't think so since of all these functions are new. These seem 
> like
>     Daniel> simple internal refactorings. Adding a NOERROR argument to 
> key-parse
>     Daniel> seems like the least intrusive approach.
> 
> I know emacs-29 hasnʼt been released yet, but changing a function to
> error by default when it didnʼt do previously seems risky. 

It is mostly used internally. There are only 9 call sites in the Emacs code.

> Iʼll make
> that change locally and see what happens. (Update: it did not go well,
> there are test-suite failures).

This is hardly an argument. You should check all the call sites and
adjust accordingly. In particular `kbd' must pass 'noerror. I would
expect this to be a pretty small patch given the small number of call sites.

Daniel