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

From: Ruijie Yu <[email-masked]>
Date: Tue, 09 May 2023 12:47:51 +0800

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.

>From 29989c7e6be3bd3bd58412273f2516f23058e152 Mon Sep 17 00:00:00 2001
From: Ruijie Yu <[email-masked]>
Date: Tue, 9 May 2023 10:47:51 +0800
Subject: [PATCH v1] Change eshell defcustom types and minor fixes

* lisp/eshell/em-alias.el (eshell-bad-command-tolerance):
* lisp/eshell/em-dirs.el (eshell-last-dir-ring-size):
* lisp/eshell/em-ls.el (eshell-ls-default-blocksize):
* lisp/eshell/esh-ext.el (eshell-command-interpreter-max-length):
* lisp/eshell/esh-io.el (eshell-error-handle):
(eshell-number-of-handles): (eshell-output-handle):
(eshell-print-queue-size):
* lisp/eshell/esh-proc.el (eshell-kill-process-wait-time):
(eshell-process-wait-milliseconds): (eshell-process-wait-seconds):
* lisp/eshell/esh-util.el (eshell-private-directory-modes):
(eshell-private-file-modes): Change integer types into natnum
types, because logically these values must be nonnegative.

* lisp/eshell/em-cmpl.el (eshell-command-completions-alist):
* lisp/eshell/em-hist.el (eshell-hist-rebind-keys-alist):
* lisp/eshell/em-ls.el (eshell-ls-highlight-alist):
* lisp/eshell/em-pred.el (eshell-modifier-alist):
(eshell-predicate-alist):
* lisp/eshell/em-term.el (eshell-visual-options):
(eshell-visual-subcommands):
* lisp/eshell/esh-ext.el (eshell-interpreter-alist): Change
customization type (repeat (cons K V)) into (alist :key-type K
:value-type V) to be more descriptive.  Supply tags for some of
these variables.

* lisp/eshell/em-hist.el (eshell-input-filter):
* lisp/eshell/esh-arg.el (eshell-parse-argument-hook): Change
symbol quotes to function quotes where trivially possible.

* lisp/eshell/esh-cmd.el (eshell-subcommand-bindings): Change
plain sexp type into a more precise type for what a let-binding
should be.  (Does it warrant a `define-widget'?)

* lisp/eshell/em-glob.el (eshell-glob-translate-alist): Unquote
lambdas to allow for compilation.

* lisp/eshell/em-hist.el (eshell-hist-ignoredups):
* lisp/eshell/esh-mode.el (eshell-scroll-to-bottom-on-input):
(eshell-scroll-to-bottom-on-output): Change const to other to
follow docstring pedantically.  [Note the TODO in esh-mode.el.]

* lisp/eshell/esh-ext.el (eshell-force-execution):
* lisp/eshell/esh-mode.el (eshell-buffer-maximum-lines): Fix
docstring typos.

[TODO]

* lisp/eshell/em-cmpl.el (eshell-command-completion-function):
* lisp/eshell/em-cmpl.el (eshell-default-completion-function):
* lisp/eshell/em-glob.el (eshell-glob-translate-alist):
* lisp/eshell/em-hist.el (eshell-hist-unload-hook):
* lisp/eshell/em-pred.el (eshell-predicate-alist):
(eshell-modifier-alist):
* lisp/eshell/em-prompt.el (eshell-prompt-function):
* lisp/eshell/em-smart.el (eshell-smart-unload-hook):
* lisp/eshell/esh-arg.el (eshell-parse-argument-hook): TODO: Do we
need to convert lambdas to named internal helper functions?  Note
that some of these variables are marked as obsolete.

* lisp/eshell/em-hist.el (eshell-hist-rebind-keys-alist):
* lisp/eshell/em-rebind.el (eshell-rebind-keys-alist): TODO: This
customization type is used multiple times, and may warrant using
`define-widget'.

* lisp/eshell/esh-arg.el (eshell-special-chars-outside-quoting):
TODO: Do we need a :set function here to append on change?

* lisp/eshell/esh-mode.el (eshell-password-prompt-regexp): TODO:
Do we need to let-bind the result of `apply' and re-use it twice
when calling `format'?  This would save one call to `apply'.

* lisp/eshell/esh-util.el (eshell-group-file):
(eshell-passwd-file): (eshell-hosts-file): TODO: Should we require
the file to exist?
(eshell-private-file-modes): TODO: The "file mode" customization
type is used multiple times, and may be a general enough type for
more packages.  In this case, does it warrant a file-mode type in
wid-edit.el?

* lisp/eshell/esh-util.el (eshell-ange-ls-uids): TODO: docstring
unclear, saying it should be list of strings, but type says it is
an alist from string to nonempty list of strings.

* lisp/eshell/esh-cmd.el (eshell-subcommand-bindings): TODO: The
customization type to describe a let-binding is used multiple
times.  Is it worth it to define a let-binding type in
wid-edit.el?  If not, maybe we should define in eshell anyways to
avoid code duplication.
---
 lisp/eshell/em-alias.el  |  2 +-
 lisp/eshell/em-cmpl.el   |  4 +++-
 lisp/eshell/em-dirs.el   |  2 +-
 lisp/eshell/em-glob.el   | 19 ++++++++++---------
 lisp/eshell/em-hist.el   | 14 +++++++++-----
 lisp/eshell/em-ls.el     |  8 ++++++--
 lisp/eshell/em-pred.el   |  7 +++++--
 lisp/eshell/em-prompt.el |  1 +
 lisp/eshell/em-rebind.el |  6 +++++-
 lisp/eshell/em-smart.el  |  1 +
 lisp/eshell/em-term.el   |  8 ++++----
 lisp/eshell/esh-arg.el   | 16 +++++++++++-----
 lisp/eshell/esh-cmd.el   |  5 ++++-
 lisp/eshell/esh-ext.el   |  9 +++++----
 lisp/eshell/esh-io.el    |  8 ++++----
 lisp/eshell/esh-mode.el  | 14 +++++++++++---
 lisp/eshell/esh-proc.el  |  6 +++---
 lisp/eshell/esh-util.el  |  9 +++++++--
 18 files changed, 91 insertions(+), 48 deletions(-)

diff --git a/lisp/eshell/em-alias.el b/lisp/eshell/em-alias.el
index 841982c3425..a1530b0842b 100644
--- a/lisp/eshell/em-alias.el
+++ b/lisp/eshell/em-alias.el
@@ -112,7 +112,7 @@ eshell-aliases-file
 
 (defcustom eshell-bad-command-tolerance 3
   "The number of failed commands to ignore before creating an alias."
-  :type 'integer
+  :type 'natnum
   ;; :link '(custom-manual "(eshell)Auto-correction of bad commands")
   :group 'eshell-alias)
 
diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index 732bbb3f1fa..ede20d53a25 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -131,7 +131,7 @@ eshell-command-completions-alist
   "An alist that defines simple argument type correlations.
 This is provided for common commands, as a simplistic alternative
 to writing a completion function."
-  :type '(repeat (cons string regexp)))
+  :type '(alist :key-type string :value-type regexp))
 
 (defun eshell-cmpl--custom-variable-docstring (pcomplete-var)
   "Generate the docstring of a variable derived from a pcomplete-* variable."
@@ -184,6 +184,7 @@ eshell-cmpl-restore-window-delay
   (eshell-cmpl--custom-variable-docstring 'pcomplete-restore-window-delay)
   :type (get 'pcomplete-restore-window-delay 'custom-type))
 
+;; TODO: convert λ into named function?
 (defcustom eshell-command-completion-function
   (lambda ()
     (pcomplete-here (eshell--complete-commands-list)))
@@ -195,6 +196,7 @@ eshell-cmpl-command-name-function
   (eshell-cmpl--custom-variable-docstring 'pcomplete-command-name-function)
   :type (get 'pcomplete-command-name-function 'custom-type))
 
+;; TODO: convert λ into named function?
 (defcustom eshell-default-completion-function
   (lambda ()
     (while (pcomplete-here
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)
 
 (defcustom eshell-last-dir-unique t
   "If non-nil, `eshell-last-dir-ring' contains only unique entries."
diff --git a/lisp/eshell/em-glob.el b/lisp/eshell/em-glob.el
index d00f8c93cd1..9471003eb7f 100644
--- a/lisp/eshell/em-glob.el
+++ b/lisp/eshell/em-glob.el
@@ -101,8 +101,9 @@ eshell-glob-chars-list
   :type '(repeat character)
   :group 'eshell-glob)
 
+;; TODO: convert the λ into named function?
 (defcustom eshell-glob-translate-alist
-  '((?\] . "]")
+  `((?\] . "]")
     (?\[ . "[")
     (?^  . "^")
     (??  . ".")
@@ -111,14 +112,14 @@ eshell-glob-translate-alist
     (?\( . "\\(")
     (?\) . "\\)")
     (?\| . "\\|")
-    (?#  . (lambda (str pos)
-	     (if (and (< (1+ pos) (length str))
-		      (memq (aref str (1+ pos)) '(?* ?# ?+ ??)))
-		 (cons (if (eq (aref str (1+ pos)) ??)
-			   "?"
-			 (if (eq (aref str (1+ pos)) ?*)
-			     "*" "+")) (+ pos 2))
-	       (cons "*" (1+ pos))))))
+    (?#  . ,(lambda (str pos)
+	      (if (and (< (1+ pos) (length str))
+		       (memq (aref str (1+ pos)) '(?* ?# ?+ ??)))
+		  (cons (cond ((eq (aref str (1+ pos)) ??) "?")
+                              ((eq (aref str (1+ pos)) ?*) "*")
+                              (t "+"))
+                        (+ pos 2))
+	        (cons "*" (1+ pos))))))
   "An alist for translation of extended globbing characters."
   :type '(alist :key-type character
 		:value-type (choice string function))
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
@@ -74,6 +74,7 @@ eshell-hist-load-hook
   :version "24.1"			; removed eshell-hist-initialize
   :type 'hook)
 
+;; TODO: named function?  Maybe not, obsolete function.
 (defcustom eshell-hist-unload-hook
   (list
    (lambda ()
@@ -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)))
 
 (defcustom eshell-save-history-on-exit t
@@ -116,7 +117,7 @@ eshell-save-history-on-exit
 		 (const :tag "Ask" ask)
 		 (const :tag "Always save" t)))
 
-(defcustom eshell-input-filter 'eshell-input-filter-default
+(defcustom eshell-input-filter #'eshell-input-filter-default
   "Predicate for filtering additions to input history.
 Takes one argument, the input.  If non-nil, the input may be saved on
 the input history list.  Default is to save anything that isn't all
@@ -171,6 +172,7 @@ eshell-hist-modifier
   "The regexp used to identity history modifiers."
   :type 'regexp)
 
+;; TODO: this type is used in multiple places.
 (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")))
 
 ;;; Internal Variables:
 
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
@@ -86,7 +86,7 @@ eshell-ls-use-in-dired
 
 (defcustom eshell-ls-default-blocksize 1024
   "The default blocksize to use when display file sizes with -s."
-  :type 'integer)
+  :type 'natnum)
 
 (defcustom eshell-ls-exclude-regexp nil
   "Unless -a is specified, files matching this regexp will not be shown."
@@ -214,6 +214,7 @@ eshell-ls-applicable
 	    ;; Otherwise call FUNC.
 	    (,(eval func) ,file)))))
 
+;; TODO: original type was wrong?
 (defcustom eshell-ls-highlight-alist nil
   "This alist correlates test functions to color.
 The format of the members of this alist is
@@ -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))
 
 (defvar block-size)
 (defvar dereference-links)
diff --git a/lisp/eshell/em-pred.el b/lisp/eshell/em-pred.el
index bfb0dad60ef..fa462e0478e 100644
--- a/lisp/eshell/em-pred.el
+++ b/lisp/eshell/em-pred.el
@@ -65,6 +65,7 @@ eshell-pred-load-hook
   :version "24.1"			; removed eshell-pred-initialize
   :type 'hook)
 
+;; TODO: convert λ's here into named functions?
 (defcustom eshell-predicate-alist
   '((?/ . (eshell-pred-file-type ?d))   ; directories
     (?. . (eshell-pred-file-type ?-))   ; regular files
@@ -107,9 +108,11 @@ eshell-predicate-alist
 The format of each entry is
 
   (CHAR . PREDICATE-FUNC-SEXP)"
-  :type '(repeat (cons character sexp))
+  :type '(alist :key-type character :value-type sexp)
   :risky t)
 
+;; TODO: convert λ's here into named functions?  Or at least allow
+;; compiler to compile the λ's.
 (defcustom eshell-modifier-alist
   '((?E . (lambda (lst) (mapcar #'eshell-eval-argument lst)))
     (?L . (lambda (lst) (mapcar #'downcase lst)))
@@ -138,7 +141,7 @@ eshell-modifier-alist
 The format of each entry is
 
   (CHAR . MODIFIER-FUNC-SEXP)"
-  :type '(repeat (cons character sexp))
+  :type '(alist :key-type character :value-type sexp)
   :risky t)
 
 (defvar eshell-predicate-help-string
diff --git a/lisp/eshell/em-prompt.el b/lisp/eshell/em-prompt.el
index 9f9e58e83d7..2bebf5f0bf6 100644
--- a/lisp/eshell/em-prompt.el
+++ b/lisp/eshell/em-prompt.el
@@ -47,6 +47,7 @@ eshell-prompt-load-hook
 
 (autoload 'eshell/pwd "em-dirs")
 
+;; TODO: convert λ into named function?
 (defcustom eshell-prompt-function
   (lambda ()
     (concat (abbreviate-file-name (eshell/pwd))
diff --git a/lisp/eshell/em-rebind.el b/lisp/eshell/em-rebind.el
index 75a2848a9d5..c4df2a5df78 100644
--- a/lisp/eshell/em-rebind.el
+++ b/lisp/eshell/em-rebind.el
@@ -48,6 +48,8 @@ eshell-rebind-load-hook
   :type 'hook
   :group 'eshell-rebind)
 
+;; TODO: this type is used multiple times.  Is it worth it to extract
+;; it into a `define-widget'?
 (defcustom eshell-rebind-keys-alist
   '(([(control ?d)] . eshell-delchar-or-maybe-eof)
     ([backspace]    . eshell-delete-backward-char)
@@ -55,8 +57,10 @@ eshell-rebind-keys-alist
     ([(control ?w)] . backward-kill-word)
     ([(control ?u)] . eshell-kill-input))
   "Bind some keys differently if point is in input text."
+  ;; Waiting for response on the TODO.  The other place has changes to
+  ;; :type.
   :type '(repeat (cons (vector :tag "Keys to bind"
-			       (repeat :inline t sexp))
+                               (repeat :inline t sexp))
 		       (function :tag "Command")))
   :group 'eshell-rebind)
 
diff --git a/lisp/eshell/em-smart.el b/lisp/eshell/em-smart.el
index d8b7fadc2c2..7110e56ee9f 100644
--- a/lisp/eshell/em-smart.el
+++ b/lisp/eshell/em-smart.el
@@ -91,6 +91,7 @@ eshell-smart-load-hook
   :type 'hook
   :group 'eshell-smart)
 
+;; TODO: λ's to helper function?  Obsolete defcustom though.
 (defcustom eshell-smart-unload-hook
   (list
    (lambda ()
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index ab26da857b7..a8778abeacc 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -85,8 +85,8 @@ eshell-visual-subcommands
 because git shows logs and diffs using a pager by default.
 
 See also `eshell-visual-commands' and `eshell-visual-options'."
-  :type '(repeat (cons (string :tag "Command")
-		       (repeat (string :tag "Subcommand"))))
+  :type '(alist :key-type (string :tag "Command")
+                :value-type (repeat (string :tag "Subcommand")))
   :version "24.4")
 
 (defcustom eshell-visual-options
@@ -106,8 +106,8 @@ eshell-visual-options
 always uses a pager for output.
 
 See also `eshell-visual-commands' and `eshell-visual-subcommands'."
-  :type '(repeat (cons (string :tag "Command")
-		       (repeat (string :tag "Option"))))
+  :type '(alist :key-type (string :tag "Command")
+                :value-type (repeat (string :tag "Option")))
   :version "24.4")
 
 ;; If you change this from term-term-name, you need to ensure that the
diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el
index aa1e8f77ea5..2b8182dfc41 100644
--- a/lisp/eshell/esh-arg.el
+++ b/lisp/eshell/esh-arg.el
@@ -70,6 +70,10 @@ eshell-special-chars-inside-quoting
   :type '(repeat character)
   :group 'eshell-arg)
 
+;; TODO: do we need to add a :set function to auto-append
+;; `eshell-delimiter-argument-list'?  What if entries are removed from
+;; `eshell-delimiter-argument-list'?  We can also not care and keep
+;; this as-is.
 (defcustom eshell-special-chars-outside-quoting
   (append eshell-delimiter-argument-list '(?# ?! ?\\ ?\" ?\'))
   "Characters that require escaping outside of double quotes.
@@ -84,11 +88,13 @@ eshell-arg-delimiter
     (or (= pos (point-max))
 	(memq (char-after pos) eshell-delimiter-argument-list))))
 
+;; TODO: a lot of λ's here.  Do we need to convert them into named
+;; (internal, helper) functions?
 (defcustom eshell-parse-argument-hook
   (list
    ;; a term such as #<buffer NAME>, or #<process NAME> is a buffer
    ;; or process reference
-   'eshell-parse-special-reference
+   #'eshell-parse-special-reference
 
    ;; numbers convert to numbers if they stand alone
    (lambda ()
@@ -135,16 +141,16 @@ eshell-parse-argument-hook
          (eshell-finish-arg))))
 
    ;; parse backslash and the character after
-   'eshell-parse-backslash
+   #'eshell-parse-backslash
 
    ;; text beginning with ' is a literally quoted
-   'eshell-parse-literal-quote
+   #'eshell-parse-literal-quote
 
    ;; text beginning with " is interpolably quoted
-   'eshell-parse-double-quote
+   #'eshell-parse-double-quote
 
    ;; argument delimiter
-   'eshell-parse-delimiter)
+   #'eshell-parse-delimiter)
   "Define how to process Eshell command line arguments.
 When each function on this hook is called, point will be at the
 current position within the argument list.  The function should either
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 94aa2ed8906..895f7951baa 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -258,13 +258,16 @@ eshell-deferrable-commands
 the command."
   :type '(repeat function))
 
+;; TODO: let-binding used multiple times.  Convert to `define-widget'?
 (defcustom eshell-subcommand-bindings
   '((eshell-in-subcommand-p t)
     (eshell-in-pipeline-p nil)
     (default-directory default-directory))
   "A list of `let' bindings for subcommand environments."
   :version "29.1"		       ; removed `process-environment'
-  :type 'sexp
+  :type '(repeat (choice (symbol :tag "Bind symbol to nil")
+                         (list (symbol :tag "Symbol name")
+                               (sexp :tag "Bound value"))))
   :risky t)
 
 (defvar eshell-ensure-newline-p nil
diff --git a/lisp/eshell/esh-ext.el b/lisp/eshell/esh-ext.el
index f350622e78c..dca04669c02 100644
--- a/lisp/eshell/esh-ext.el
+++ b/lisp/eshell/esh-ext.el
@@ -63,7 +63,7 @@ eshell-force-execution
 This can be useful on systems like Windows, where the operating system
 doesn't support the execution bit for shell scripts; or in cases where
 you want to associate an interpreter with a particular kind of script
-file, but the language won't let you but a `#!' interpreter line in
+file, but the language won't let you put a `#!' interpreter line in
 the file, and you don't want to make it executable since nothing else
 but Eshell will be able to understand
 `eshell-interpreter-alist'."
@@ -139,8 +139,9 @@ eshell-interpreter-alist
 functions should throw `eshell-replace-command' with the alternate
 command form, or they should return a value compatible with the
 possible return values of `eshell-external-command', which see."
-  :type '(repeat (cons (choice regexp (function :tag "Predicate"))
-		       (choice string (function :tag "Interpreter"))))
+  :type '(alist
+          :key-type (choice regexp (function :tag "Predicate"))
+	  :value-type (choice string (function :tag "Interpreter")))
   :group 'eshell-ext)
 
 (defcustom eshell-alternate-command-hook nil
@@ -158,7 +159,7 @@ eshell-alternate-command-hook
 
 (defcustom eshell-command-interpreter-max-length 256
   "The maximum length of any command interpreter string, plus args."
-  :type 'integer
+  :type 'natnum
   :group 'eshell-ext)
 
 (defcustom eshell-explicit-command-char ?*
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index cccdb49ce2a..1167b00f55a 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -97,24 +97,24 @@ eshell-number-of-handles
 these Emacs does not currently support with asynchronous processes
 \(which is what eshell uses so that you can continue doing work in
 other buffers)."
-  :type 'integer
+  :type 'natnum
   :group 'eshell-io)
 
 (defcustom eshell-output-handle 1
   "The index of the standard output handle."
-  :type 'integer
+  :type 'natnum
   :group 'eshell-io)
 
 (defcustom eshell-error-handle 2
   "The index of the standard error handle."
-  :type 'integer
+  :type 'natnum
   :group 'eshell-io)
 
 (defcustom eshell-print-queue-size 5
   "The size of the print queue, for doing buffered printing.
 This is basically a speed enhancement, to avoid blocking the Lisp code
 from executing while Emacs is redisplaying."
-  :type 'integer
+  :type 'natnum
   :group 'eshell-io)
 
 (defcustom eshell-virtual-targets
diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index 0c381dbb86a..a615809ef4d 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -124,7 +124,12 @@ eshell-scroll-to-bottom-on-input
 
 See `eshell-preinput-scroll-to-bottom'."
   :type '(radio (const :tag "Do not scroll Eshell windows" nil)
-		(const :tag "Scroll all windows showing the buffer" all)
+                ;; TODO: this looks weird, but matches exactly what
+                ;; docstring says.  Should we update docstring to say
+                ;; that anything unrecognized is considered "all", and
+                ;; we can use (other :tag "Tag" all) here?
+		(choice :tag "Scroll all windows showing the buffer"
+                        (const all) (const t))
                 (const :tag "Scroll only the selected window" this)))
 
 (defcustom eshell-scroll-to-bottom-on-output nil
@@ -136,7 +141,9 @@ eshell-scroll-to-bottom-on-output
 See variable `eshell-scroll-show-maximum-output' and function
 `eshell-postoutput-scroll-to-bottom'."
   :type '(radio (const :tag "Do not scroll Eshell windows" nil)
-		(const :tag "Scroll all windows showing the buffer" all)
+                ;; TODO: see `eshell-scroll-to-bottom-on-input'.
+                (choice :tag "Scroll all windows showing the buffer"
+                        (const all) (const t))
 		(const :tag "Scroll only the selected window" this)
                 (const :tag "Scroll all windows other than selected" others)))
 
@@ -151,7 +158,7 @@ eshell-scroll-show-maximum-output
 (defcustom eshell-buffer-maximum-lines 1024
   "The maximum size in lines for eshell buffers.
 Eshell buffers are truncated from the top to be no greater than this
-number, if the function `eshell-truncate-buffer' is on
+number, if the function `eshell-truncate-buffer' is in
 `eshell-output-filter-functions'."
   :type 'natnum)
 
@@ -172,6 +179,7 @@ eshell-preoutput-filter-functions
 inserted.  They return the string as it should be inserted."
   :type 'hook)
 
+;; TODO: let-bind result of `apply'?
 (defcustom eshell-password-prompt-regexp
   (format "%s[^%s]*[%s]\\s *\\'"
           (regexp-opt password-word-equivalents t)
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index 00e0c8014e1..838306cb906 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -42,11 +42,11 @@ eshell-proc-load-hook
 
 (defcustom eshell-process-wait-seconds 0
   "The number of seconds to delay waiting for a synchronous process."
-  :type 'integer)
+  :type 'natnum)
 
 (defcustom eshell-process-wait-milliseconds 50
   "The number of milliseconds to delay waiting for a synchronous process."
-  :type 'integer)
+  :type 'natnum)
 
 (defcustom eshell-done-messages-in-minibuffer t
   "If non-nil, subjob \"Done\" messages will display in minibuffer."
@@ -526,7 +526,7 @@ eshell-process-interact
 
 (defcustom eshell-kill-process-wait-time 5
   "Seconds to wait between sending termination signals to a subprocess."
-  :type 'integer)
+  :type 'natnum)
 
 (defcustom eshell-kill-process-signals '(SIGINT SIGQUIT SIGKILL)
   "Signals used to kill processes when an Eshell buffer exits.
diff --git a/lisp/eshell/esh-util.el b/lisp/eshell/esh-util.el
index 3608c78ba2b..ded020152f4 100644
--- a/lisp/eshell/esh-util.el
+++ b/lisp/eshell/esh-util.el
@@ -40,14 +40,17 @@ eshell-stringify-t
 similarly to external commands, as far as successful result output."
   :type 'boolean)
 
+;; TODO: should we require existing file?
 (defcustom eshell-group-file "/etc/group"
   "If non-nil, the name of the group file on your system."
   :type '(choice (const :tag "No group file" nil) file))
 
+;; TODO: should we require existing file?
 (defcustom eshell-passwd-file "/etc/passwd"
   "If non-nil, the name of the passwd file on your system."
   :type '(choice (const :tag "No passwd file" nil) file))
 
+;; TODO: should we require existing file?
 (defcustom eshell-hosts-file "/etc/hosts"
   "The name of the /etc/hosts file.
 Use `pcomplete-hosts-file' instead; this variable is obsolete and
@@ -63,13 +66,14 @@ eshell-handle-errors
 Setting this to nil is offered as an aid to debugging only."
   :type 'boolean)
 
+;; TODO: could wid-edit.el define a "file-mode" customization type?
 (defcustom eshell-private-file-modes #o600 ; umask 177
   "The file-modes value to use for creating \"private\" files."
-  :type 'integer)
+  :type 'natnum)
 
 (defcustom eshell-private-directory-modes #o700 ; umask 077
   "The file-modes value to use for creating \"private\" directories."
-  :type 'integer)
+  :type 'natnum)
 
 (defcustom eshell-tar-regexp
   "\\.t\\(ar\\(\\.\\(gz\\|bz2\\|xz\\|Z\\)\\)?\\|gz\\|a[zZ]\\|z2\\)\\'"
@@ -94,6 +98,7 @@ eshell-convert-numeric-arguments
 argument matches `eshell-number-regexp'."
   :type 'boolean)
 
+;; TODO: docstring is unclear; maybe change to an alist type?
 (defcustom eshell-ange-ls-uids nil
   "List of user/host/id strings, used to determine remote ownership."
   :type '(repeat (cons :tag "Host for User/UID map"
-- 
2.40.1

-- 
Best,

RY