GNU bug report logs - #63385
30.0.50; [DRAFT PATCH v1] Update eshell defcustom definitions

Previous Next

Full log

Message #14 received at 63385 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Ruijie Yu <ruijie <at> netyu.xyz>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 63385 <at> debbugs.gnu.org,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#63385: 30.0.50; [DRAFT PATCH v1] Update eshell defcustom
 definitions
Date: Sat, 2 Sep 2023 08:33:13 -0300

Ruijie Yu <ruijie <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.