bug#35771: [PATCH] Customization type of recentf-max-saved-items

[Drew Adams <drew.adams@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 10857 bytes --]

> >> I don't know whether this has been discussed before, but it seems more
> >> suited for emacs-devel or its own bug report, rather than the current
> >> one.
> >
> > Well, it certainly also applies to this bug report, I think,
> > which is purportedly about the "Customization _type_ of
> > recentf-max-saved-items".
> 
> Sure, but it applies also to all other user options of a similar nature,

Irrelevant here.  This is no different from
suggesting clearer or more correct doc-string
wording or fixing an off-by-one error.

It pertains to _this_ bug.  Whether there might
be other, similar bugs is irrelevant to whether
it should be taken into account for fixing this one.

> and concerns a potential change in general Elisp conventions,

What potential change would that be?  Is there some
existing Elisp convention that says :type should be
mistyped or should be the loosest possible type in
preference to the most accurate type?  Does some
convention recommend omitting :type altogether, to
keep things simple?

> so I would rather it were discussed on its own
> and with other people included, and any conclusions
> reported as wishlists and/or documented.

Don't know what you're aiming for.  There's no
reason not to fix this bugged occurrence just
because you also see the possibility that similar
problems can exist elsewhere.

I already provided simple code to fix this one.
What's the problem?  Why not help users now by
letting Customize properly support the allowable/
expected set of values for this option?

> > see, again, the Subject line - why not fix it right?
> 
> This bug report is about fixing the custom :type to include nil as an
> accepted value, which the submitted patch fixes in a way suitable for
> inclusion in emacs-26.  I would rather we dealt with one issue at a
> time.

Then please fix it properly in a second step, if you
prefer.  There's no need for you (or me) to file
another report to get the customization type fixed
as it should be for this option.

> in this case there is nothing wrong with using the integer type.

Nothing wrong with using `number' either, in that case.
Nothing wrong with using `sexp' either, in that case.
If you don't want to specify the type then don't specify
the type - anything at all will do: nothing wrong, as
you say.

To me that flies in the face of why we even have :type.

But hey - we _don't_ have :type!  It's optional.  Who
needs pesky old :type?  Do as you like, including doing
nothing.

> > Use of `restricted-sexp' should be encouraged when it's
> > _needed_, and that's when the type is not currently as
> > restrictive as it should be AND there is no simpler way
> > to define the more accurate type.
> >
> > That's the point.  What we have today is not people
> > avoiding/discouraging use of `restricted-sexp' in
> > favor of just-as-useful, just-as-accurate, but simpler
> > :type specs.  We have people not using `restricted-sexp'
> > out of ignorance of it, or perhaps out of laziness.
> > (That's my guess, until convinced otherwise.)
> 
> FWIW, I am neither ignorant of it, nor too lazy to use it; rather, in my
> limited experience, I have yet to author or review a case where it is
> truly "needed", this report included.

Tautology, if you define "truly needed" by never needed
at all, which seems to be your point of view.

But if that's not really your view, what would you say
is "needed" in the attached cases (from my code)?  `sexp'?
Something more than `sexp', but avoiding `restricted-sexp'
(what)?  Would you submit a bug report for each case, to
add new simple types to avoid use of `restricted-sexp'?

When do you think `restricted-sexp' should be used?
It sounds like the answer is "never".

Since Lisp does not have typed variables (it does have
typed values, of course), I suppose you'd just rely on
the doc string as sole help for users trying to customize
the value.  Is that it?

> Existing precedent says the integer type is fine even when dealing with
> nonnegative sizes.  If you prefer to use a more accurate natnum type in
> these cases, which I sympathise with, then please submit a feature
> request for this, if one does not already exist.  I think it is wrong to
> start using restricted-sexp to emulate a natnum type in an ad hoc way.

With that point of view the `sexp' type is fine when
dealing with _any_ defcustom.  It will surely help
avoid the danger of "overspecifying".  Go for it.

IMO "existing precedent" when it comes to defcustom
is sometimes not so great.  Just like some developers
don't spend enough attention on doc strings, so some
don't spend enough attention on defcustom types.
(This bug report is a case in point, no?)

That's one reason users give up on using Customize:
it's too often not so helpful (no completion when
completion could help, for example).  We've fixed
some such oversights in the past, but there are
likely more.

Emacs developers themselves have been clear now and
then that they mostly don't use Customize, and that
shows in the lack of love and attention they give
defcustoms sometimes.  Emacs can help users more.

> > As for "not everyone uses Customize" - that's irrelevant
> > here.  This is about those who do use it, obviously.
> > It's about the :type spec of a defcustom.
> 
> It is not irrelevant.  First, authors cannot rely on the custom :type
> alone to tell users what qualifies as an expected value; 

That has nothing to do with "not everyone uses
Customize".  Even if everyone did use Customize you
would not be able to rely on :type alone to tell
users what values are acceptable.

And no one has said that one can, or should be able
to, rely on :type alone.  Totally a red herring.
You may not see it as irrelevant, but I do.

> the docstring must also contain this information.

You make it sound like having to describe the type
of the option is an unfortunate _extra_ thing that
authors have to do.  It's not.  A doc string is a
normal part of defun, defvar, defconst, defmacro,
etc.

(Just because `interactive' might control input
values, that doesn't mean that we don't need to
also document them in the doc string.  Code
controlling things properly doesn't obviate the
need for user help.  Nothing replaces doc strings.)

Having to describe the accepted value types in
the doc string is a red herring - unrelated to
the separate need to specify a proper :type.  In
one case you're talking directly to the user.  In
the other case you're communicating to Customize
(which in turn talks to the user in its own way).

> This encourages writing better docstrings 

What?  What encourages writing better doc strings?
Not having good :type values?  By that logic `sexp'
will be ideal as :type, _really_ encouraging good
doc strings.  Just don't use :type at all - get
great doc.

> and is how users may know not to set recentf-max-saved-items
> to a negative number, regardless of whether its custom :type is integer
> or natnum.  Emacs customisations have worked fine like this until now.

Again, if your argument is that doc strings alone
suffice and are the best way or the only good way
to specify the type, then :type 'sexp is called for
in all cases (or just don't use :type ever, which
amounts to the same thing).

> Second, application code must work correctly regardless of the custom
> :type.

Again, irrelevant.  Of course it must work correctly.
Doc strings are needed anyway.  And code must work
correctly anyway.  So what?  How does either of those
requirements suggest that we don't need to tell
Customize what the :type is?

> Since Elisp is not a strongly typed language, the programmer can
> only assume that the user has understood the docstring and hasn't set
> the user option to a silly value.

Why do you think defcustom has a :type at all?  Was
adding that just misguided "overspecifying" by some
overeager implementor?

> > More accurate defcustoms, using more appropriate :type
> > specs, and sometimes using :set etc., is something we
> > should encourage.  Customize and defcustoms could use
> > more love by Emacs developers, not less.
> 
> As long as "more love" means smarter, not more, use, then I agree.

It means using :type to specify the relevant/good
values; :set to specify any special behavior needed
each time it is set; :init to specify any special
behavior needed when it's initialized; correct and
complete doc strings to help users understand what
the option is for - what the option does/means; and
so on.

That you _can_ get away with specifying a minimal
:type is not a reason to do so.  That Lisp variables
are untyped is not a reason not to specify and
document the expected/allowed values.

> > Using an accurate :type spec doesn't limit/hurt users.
> > It helps them.  Likewise, using a widget edit field
> > that provides completion when appropriate etc.
> 
> Agreed.

A good start.

> >> FWIW, my vote is against both trying to overspecify custom types, and
> >> using restricted-sexp for such simple examples.
> >
> > "Overspecify"?  "Trying to overspecify"?  Please elaborate.
> > The value should be a natural number (or perhaps a positive
> > integer), no?  How is specifying that exactly overspecifying?
> > Specifying `integer' is underspecifying.  You have that
> > exactly backward.
> 
> No, I'm voting against the general notion of trying to specify more than
> is necessary, just because we can.

Define "necessary".  Lisp variables being untyped,
and especially given a doc string that specifies
the type (expected/allowed values), wanting to be
strictly and minimally "necessary", which I guess
is what you espouse, calls for :type 'sexp in all
cases (i.e., never use :type at all).

No danger, if you do that, of accidentally writing
the wrong expression for :type and introducing a
bug.  No need for anything more complex, no
"overspecifying", since the doc string does all
that's needed.  Go for it.

Oh, and since not everyone uses Customize, no real
need to use defcustom at all - just use defvar in
all cases.  No danger of a miscoded :set, no
confusing users with the Customize UI - LOTS of
nasty problems evacuated.  Go for it.

> > I don't object to adding a `natnum' :type - I suggested it.
> > But I also don't have a problem with expressing the same
> > thing even if we don't have such a type.  I think it's silly
> > to _not_ specify such behavior, and to just use `integer' (or
> > `sexp') simply because we don't have a `natnum'.  That makes
> > no sense to me.
> 
> Then please submit a report for that, if one does not already exist.

Just use :type 'sexp (or just omit :type).
Easier for everyone.  KISS, as you say.

[-- Attachment #2: foo.el --]
[-- Type: application/octet-stream, Size: 4166 bytes --]

(defcustom bar ()
  "List of custom themes for..."
  :type '(repeat (restricted-sexp
                  :match-alternatives
                  ((lambda (symb) (memq symb (custom-available-themes))))))
  :group 'foobar)

(defcustom foo ()
  "...
The option value is a list.  Each element defines a submenu or a menu
item.  A null element (`nil') is ignored.

Several alternative entry formats are available.  When customizing,
choose an alternative in the Customize `Value Menu'.

In this description:
 SYMBOL      is a symbol identifying the menu entry.
 `menu-item' is just that text, literally.
 NAME        is a string naming the menu item or submenu.
 COMMAND     is the command to be invoked by an item.
 MENU-KEYMAP is a menu keymap or a var whose value is a menu keymap.
 KEYWORDS    is a property list of menu keywords (`:enable',
             `:visible', `:filter', `:keys', etc.).

1. Single menu item.  For a selectable item, use
   (SYMBOL menu-item NAME COMMAND . KEYWORDS).  For a non-selectable
   item such as a separator, use (SYMBOL NAME) or
   (SYMBOL menu-item NAME nil . KEYWORDS).

2. Items taken from a menu-keymap variable, such as
   `menu-bar-edit-menu'.  Just use the name of the variable (a
   symbol).  The items appear at the top level of the popup menu, not
   in a submenu.

3. Submenu.  Use (SYMBOL menu-item NAME MENU-KEYMAP . KEYWORDS) or
   (SYMBOL NAME . MENU-KEYMAP).  Remember that MENU-KEYMAP can also be
   a variable (symbol) whose value is a menu keymap.

All of these are standard menu elements, with the exception of the use
of a keymap variable to represent its value.

See also:
 * (elisp) Format of Keymaps
 * (elisp) Classifying Events
 * (elisp) Extended Menu Items

Example submenu element:
 (toto menu-item \"Toto\" menu-bar-toto-menu)

Example selectable menu-item element:
 (foo menu-item \"Foo\"   foo-command
       :visible (not buffer-read-only))"
  :type  '(repeat
           (choice
            ;; These could be combined, but it's better for users to see separate choices.
            (restricted-sexp
             :tag "Submenu (SYMBOL menu-item NAME MENU-KEYMAP . KEYWORDS) or (SYMBOL NAME . MENU-KEYMAP)"
             :match-alternatives
             ((lambda (x)
                (and (consp x) (symbolp (car x))
                     (or (and (stringp (cadr x)) (cddr x)) ; (SYMBOL NAME . MENU-KEYMAP)
                         ;; (SYMBOL menu-item NAME MENU-KEYMAP . KEYWORDS)
                         (and (eq 'menu-item (cadr x))
                              (stringp (car (cddr x)))
                              (or (keymapp (car (cdr (cddr x)))) ; Can be a keymap var.
                                  (and (symbolp (car (cdr (cddr x))))
                                       (boundp (car (cdr (cddr x))))
                                       (keymapp (symbol-value (car (cdr (cddr x)))))))))))
              'nil))
            (restricted-sexp
             :tag "Items from a keymap variable's value."
             :match-alternatives ((lambda (x) (and (symbolp x)  (keymapp (symbol-value x))))
                                  'nil))
            (restricted-sexp
             :tag "Selectable item (SYMBOL menu-item NAME COMMAND . KEYWORDS)"
             :match-alternatives ((lambda (x) (and (consp x)  (symbolp (car x))
                                              (eq 'menu-item (cadr x))
                                              (stringp (car (cddr x)))
                                              (commandp (car (cdr (cddr x))))))
                                  'nil))
            (restricted-sexp
             :tag "Non-selectable item (SYMBOL NAME) or (SYMBOL menu-item NAME nil . KEYWORDS)"
             :match-alternatives ((lambda (x) (and (consp x)  (symbolp (car x))
                                              (or (and (stringp (cadr x))  (null (caddr x)))
                                                  (and (eq 'menu-item (cadr x))
                                                       (stringp (car (cddr x)))
                                                       (null (car (cdr (cddr x))))))))
                                  'nil))))
  :group 'foobar)