[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argumen
From: |
Daniel Mendler |
Subject: |
bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument |
Date: |
Thu, 19 Jan 2023 12:19:19 +0100 |
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
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Daniel Mendler, 2023/01/16
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/17
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Daniel Mendler, 2023/01/17
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/18
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Daniel Mendler, 2023/01/18
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Daniel Mendler, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Daniel Mendler, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument,
Daniel Mendler <=
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Daniel Mendler, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Eli Zaretskii, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Eli Zaretskii, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/19
- bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument, Robert Pluim, 2023/01/20