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

Previous Next

To reply to this bug, email your comments to 63385 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

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

From: Ruijie Yu <ruijie <at> netyu.xyz>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [DRAFT PATCH v1] Update eshell defcustom definitions
Date: Tue, 09 May 2023 12:47:51 +0800

[Message part 1 (text/plain, inline)]

Hello,

When migrating from my old eshell configuration into using `setopt', I
noticed a discrepency in the docstring of
`eshell-scroll-to-bottom-on-input' and its customization type (the
docstring says t and 'all should be equivalent, whereas the type does
not allow t at all), hence I started looking into fixing issues around
the `defcustom' calls in eshell.

Here attached is a draft patch to update eshell defcustoms.  The commit
message contains a long [TODO] section outlining all my ideas on how to
change a given portion, but these ideas probably need more input.

Patch based on 34ac7d90876, 4-5 days old master.

[v1-0001-Change-eshell-defcustom-types-and-minor-fixes.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Best,


RY

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Ruijie Yu <ruijie <at> netyu.xyz>, 63385 <at> debbugs.gnu.org
Subject: Re: bug#63385: 30.0.50; [DRAFT PATCH v1] Update eshell defcustom
 definitions
Date: Sun, 14 May 2023 21:39:55 -0700

On 5/8/2023 9:47 PM, Ruijie Yu via Bug reports for GNU Emacs, the Swiss 
army knife of text editors wrote:
> Here attached is a draft patch to update eshell defcustoms.  The commit
> message contains a long [TODO] section outlining all my ideas on how to
> change a given portion, but these ideas probably need more input.

Thanks, I think most of these make sense. I'm not too sure what to do 
about the parts you marked with "TODO" though; it'll take some thought 
for each of them. (For example, I feel like there's a better way to let 
users customize the keys in em-rebind.el and elsewhere.)

How about removing them from the patch, and then we can merge the remainder?

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Ruijie Yu <ruijie <at> netyu.xyz>, 63385 <at> debbugs.gnu.org
Subject: Re: bug#63385: 30.0.50;
 [DRAFT PATCH v1] Update eshell defcustom definitions
Date: Fri, 1 Sep 2023 21:33:21 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:
>
> On 5/8/2023 9:47 PM, Ruijie Yu via Bug reports for GNU Emacs, the Swiss
> army knife of text editors wrote:
> > Here attached is a draft patch to update eshell defcustoms.  The commit
> > message contains a long [TODO] section outlining all my ideas on how to
> > change a given portion, but these ideas probably need more input.
>
> Thanks, I think most of these make sense. I'm not too sure what to do
> about the parts you marked with "TODO" though; it'll take some thought
> for each of them. (For example, I feel like there's a better way to let
> users customize the keys in em-rebind.el and elsewhere.)
>
> How about removing them from the patch, and then we can merge the remainder?

Sounds good to me.

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.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: Ruijie Yu <ruijie <at> netyu.xyz>, Jim Porter <jporterbugs <at> gmail.com>,
 63385 <at> debbugs.gnu.org
Subject: Re: bug#63385: 30.0.50;
 [DRAFT PATCH v1] Update eshell defcustom definitions
Date: Tue, 11 Feb 2025 20:32:49 -0800

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> 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.

Ruijie, did you have time to look into the comments on your patch?

Previous Next

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