[PATCH] Improve find-sibling-rules option type

[PATCH] Improve find-sibling-rules option type

[Paul W. Rankin via Emacs development discussions.]

* lisp/files.el (find-sibling-rules): use alist with tags for custom
  type
---
This is preferable than having to enter a sexp as a user option.


 lisp/files.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index 9d76668..e7adccd 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -7572,7 +7572,8 @@ files, you could say something like:
 In this example, if you're in \"src/emacs/emacs-27/lisp/abbrev.el\",
 and a \"src/emacs/emacs-28/lisp/abbrev.el\" file exists, it's now
 defined as a sibling."
-  :type 'sexp
+  :type '(alist :key-type (string :tag "Match")
+                :value-type (string :tag "Expansion"))
   :version "29.1")
 
 (defun find-sibling-file (file)
-- 
2.42.0

Re: [PATCH] Improve find-sibling-rules option type

[Eli Zaretskii]

> Cc: "Paul W. Rankin" <hello@paulwrankin.com>
> Date: Sun, 24 Sep 2023 11:03:27 +1000
> From:  "Paul W. Rankin" via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> * lisp/files.el (find-sibling-rules): use alist with tags for custom
>   type
> ---
> This is preferable than having to enter a sexp as a user option.

Thanks.

Mauro, any comments?

Re: [PATCH] Improve find-sibling-rules option type

[Mauro Aranda]

On 24/9/23 02:33, Eli Zaretskii wrote:
 >> Cc: "Paul W. Rankin" <hello@paulwrankin.com>
 >> Date: Sun, 24 Sep 2023 11:03:27 +1000
 >> From:  "Paul W. Rankin" via "Emacs development discussions." 
<emacs-devel@gnu.org>
 >>
 >> * lisp/files.el (find-sibling-rules): use alist with tags for custom
 >>   type
 >> ---
 >> This is preferable than having to enter a sexp as a user option.
 >
 > Thanks.
 >
 > Mauro, any comments?

In particular, AFAICT there could be more than one EXPANSION.  In that
case, :value-type should be a repeat of strings, not just a string.  In
addition, regexp should be used as the :key-type type.  (I see that
Philip already spotted this)

In general, when converting from sexp to a more specific type, I think
we have to take extra precaution and check the code for how the
variable it's used (i.e., the docstring might not tell the whole story).
If ever in doubt, then it might be better and safer to offer a choice
with the more specific type first, and keep sexp as a catch-all
alternative.

Re: [PATCH] Improve find-sibling-rules option type

[Paul W. Rankin via Emacs development discussions.]

On 2023-09-24 19:44, Mauro Aranda wrote:
> In particular, AFAICT there could be more than one EXPANSION.  In that
> case, :value-type should be a repeat of strings, not just a string.  In
> addition, regexp should be used as the :key-type type.  (I see that
> Philip already spotted this)

Just looking at `find-sibling-file-search' and it looks like maybe? But 
if this is the case I think the docstring should be rewritten. Looking 
at it from a user's perspective if the docstring says "a string" but 
there's a repeat of strings, that would be confusing.

> In general, when converting from sexp to a more specific type, I think
> we have to take extra precaution and check the code for how the
> variable it's used (i.e., the docstring might not tell the whole 
> story).
> If ever in doubt, then it might be better and safer to offer a choice
> with the more specific type first, and keep sexp as a catch-all
> alternative.

Customize already provides for this! The user can click [State] > Show 
Saved Lisp Expression to edit/override the value with their own sexp.

I think the main thing is not to have user options that demand sexps.

Re: [PATCH] Improve find-sibling-rules option type

[Mauro Aranda]

On 24/9/23 22:17, Paul W. Rankin wrote:
 >
 > On 2023-09-24 19:44, Mauro Aranda wrote:
 >> In particular, AFAICT there could be more than one EXPANSION.  In that
 >> case, :value-type should be a repeat of strings, not just a string.  In
 >> addition, regexp should be used as the :key-type type. (I see that
 >> Philip already spotted this)
 >
 > Just looking at `find-sibling-file-search' and it looks like maybe? 
But if this is the case I think the docstring should be rewritten. 
Looking at it from a user's perspective if the docstring says "a string" 
but there's a repeat of strings, that would be confusing.

My clue was (MATCH EXPANSION...) and then it says EXPANSIONS.  And each
EXPANSION is a string, hence we have a repeat of strings.

 >> In general, when converting from sexp to a more specific type, I think
 >> we have to take extra precaution and check the code for how the
 >> variable it's used (i.e., the docstring might not tell the whole story).
 >> If ever in doubt, then it might be better and safer to offer a choice
 >> with the more specific type first, and keep sexp as a catch-all
 >> alternative.
 >
 > Customize already provides for this! The user can click [State] > 
Show Saved Lisp Expression to edit/override the value with their own sexp.

That was not my point.  My point was that in a "sexp to
more-specific-type" change, there's always a risk to introduce a minor
regression: that the user starts seeing a MISMATCH instead of SET/SAVED
as a message in Customize.  IMO, that needs to be avoided as much as
possible.

But AFAICS, there will be no trouble here, provided that :value-type is
a repeat of strings.

 > I think the main thing is not to have user options that demand sexps.

I agree.  I'm in favor of avoiding sexp as much as possible, be it by
providing a more specific type, or a not-that-complicated user option.

Re: [PATCH] Improve find-sibling-rules option type

[Philip Kaludercic]

"Paul W. Rankin" via "Emacs development discussions."
<emacs-devel@gnu.org> writes:

> * lisp/files.el (find-sibling-rules): use alist with tags for custom
>   type
> ---
> This is preferable than having to enter a sexp as a user option.
>
>
>  lisp/files.el | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/files.el b/lisp/files.el
> index 9d76668..e7adccd 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -7572,7 +7572,8 @@ files, you could say something like:
>  In this example, if you're in \"src/emacs/emacs-27/lisp/abbrev.el\",
>  and a \"src/emacs/emacs-28/lisp/abbrev.el\" file exists, it's now
>  defined as a sibling."
> -  :type 'sexp
> +  :type '(alist :key-type (string :tag "Match")
                              ^
                              (elisp) Simple Types lists a type `regexp'
                              that you could use here as well.

> +                :value-type (string :tag "Expansion"))
>    :version "29.1")
>  
>  (defun find-sibling-file (file)

-- 
Philip Kaludercic