[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#63385: 30.0.50; [DRAFT PATCH v1] Update eshell defcustom definitions
From: |
Mauro Aranda |
Subject: |
bug#63385: 30.0.50; [DRAFT PATCH v1] Update eshell defcustom definitions |
Date: |
Sat, 2 Sep 2023 08:33:13 -0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 |
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.