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

Previous Next

Full log

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.