bug#63385: 30.0.50; [DRAFT PATCH v1] Update eshell defcustom definitions

[Mauro Aranda <maurooaranda@gmail.com>]

Ruijie Yu <ruijie@netyu.xyz> writes:

I have some comments/questions about the :type proposed changes.

 > diff --git a/lisp/eshell/em-dirs.el b/lisp/eshell/em-dirs.el
 > index 5284df9ab59..375a1356af4 100644
 > --- a/lisp/eshell/em-dirs.el
 > +++ b/lisp/eshell/em-dirs.el
 > @@ -153,7 +153,7 @@ eshell-last-dir-ring-size
 >  explicitly very much, but every once in a while would like to return to
 >  a previously visited directory without having to type in the whole
 >  thing again."
 > -  :type 'integer)
 > +  :type 'natnum)

The docstring says "If non-nil ...".  Should the :type be adapted to
allow for a nil value, or should the docstring be changed?

 > diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
 > index 2c199ec160f..f9fdbde9f19 100644
 > --- a/lisp/eshell/em-hist.el
 > +++ b/lisp/eshell/em-hist.el
 > @@ -101,7 +102,7 @@ eshell-hist-ignoredups
 >  in bash, and any other non-nil value mirrors the \"ignoredups\"
 >  value."
 >    :type '(choice (const :tag "Don't ignore anything" nil)
 > -                 (const :tag "Ignore consecutive duplicates" t)
 > +                 (other :tag "Ignore consecutive duplicates" t)
 >                   (const :tag "Only keep last duplicate" erase)))

Inside a choice, the other option should come last.  Otherwise, it will
always match, even when the value set is erase, in this case.

 >  (defcustom eshell-hist-rebind-keys-alist
 >    '(([(control ?p)]   . eshell-previous-input)
 >      ([(control ?n)]   . eshell-next-input)
 > @@ -185,9 +187,11 @@ eshell-hist-rebind-keys-alist
 >      ([up]             . eshell-previous-matching-input-from-input)
 >      ([down]           . eshell-next-matching-input-from-input))
 >    "History keys to bind differently if point is in input text."
 > -  :type '(repeat (cons (vector :tag "Keys to bind"
 > -			       (repeat :inline t sexp))
 > -		       (function :tag "Command"))))
 > +  :type '(alist :key-type (vector :tag "Keys to bind"
 > +                                  (repeat :inline t sexp))
 > +                ;; TODO: isn't there a key or key-sequencec type that
 > +                ;; can be used for the purpose of the :key-type?
 > +                :value-type (function :tag "Command")))

There is a key type.  Maybe it's worth taking a look if it works well
for this option?

 > diff --git a/lisp/eshell/em-ls.el b/lisp/eshell/em-ls.el
 > index 9b53bf29559..07f2d54fb5b 100644
 > --- a/lisp/eshell/em-ls.el
 > +++ b/lisp/eshell/em-ls.el
 > @@ -223,7 +224,10 @@ eshell-ls-highlight-alist
 >  If TEST-SEXP evals to non-nil, that face will be used to highlight the
 >  name of the file.  The first match wins.  `file' and `attrs' are in
 >  scope during the evaluation of TEST-SEXP."
 > -  :type '(repeat (cons function face)))
 > +  :type
 > +  '(alist
 > +    :key-type (sexp :tag "Test sexp, `file' and `attrs' in scope")
 > +    :value-type face))

Is sexp really necessary here? I feel like it should be used as a last
resource.