bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Philipp Stephani]

After defining a minor mode with

(define-minor-mode foo-mode nil)

its docstring will be

"Toggle Foo mode on or off.
With a prefix argument ARG, enable Foo mode if ARG is
positive, and disable it otherwise.  If called from Lisp, enable
the mode if ARG is omitted or nil, and toggle it if ARG is ‘toggle’."

This appears to indicate that

(foo-mode 'banana)

should disable foo-mode, but it enables it.  I think minor modes should
simply not allow anything but integers and 'toggle for ARG, avoiding
this confusion.


In GNU Emacs 26.0.50.3 (x86_64-apple-darwin16.0.0, NS appkit-1504.00 Version 10.12 (Build 16A323))
 of 2016-10-16 built on p
Repository revision: cf566b46a6cf85c6d54d0b0db80e32ed6ae8d1ca
Windowing system distributor 'Apple', version 10.3.1504
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Making completion list...

Configured using:
 'configure --with-modules --enable-checking
 --enable-check-lisp-object-type --without-xml2'

Configured features:
RSVG IMAGEMAGICK DBUS NOTIFY ACL GNUTLS ZLIB TOOLKIT_SCROLL_BARS NS
MODULES

Important settings:
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message subr-x puny seq byte-opt gv
bytecomp byte-compile cl-extra help-mode cconv cl-loaddefs pcase cl-lib
dired dired-loaddefs format-spec rfc822 mml easymenu mml-sec
password-cache epa derived epg epg-config gnus-util rmail rmail-loaddefs
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/ns-win ns-win
ucs-normalize term/common-win tool-bar dnd fontset image regexp-opt
fringe tabulated-list newcomment elisp-mode lisp-mode prog-mode register
page menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core term/tty-colors frame cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev obarray minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote dbusbind kqueue cocoa ns multi-tty
make-network-process emacs)

Memory information:
((conses 16 206734 11456)
 (symbols 48 20333 0)
 (miscs 40 50 157)
 (strings 32 18351 5209)
 (string-bytes 1 593891)
 (vectors 16 35291)
 (vector-slots 8 676180 5448)
 (floats 8 183 64)
 (intervals 56 219 0)
 (buffers 976 12))

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Drew Adams]

> (define-minor-mode foo-mode nil)
> its docstring will be
> 
> "Toggle Foo mode on or off.
> With a prefix argument ARG, enable Foo mode if ARG is
> positive, and disable it otherwise.  If called from Lisp, enable
> the mode if ARG is omitted or nil, and toggle it if ARG is ‘toggle’."
> 
> This appears to indicate that (foo-mode 'banana)
> should disable foo-mode, but it enables it.

No, it does not suggest that.  But to be clearer, it should
probably explicitly address the non-nil and non-`toggle' case,
like so:

  If called from Lisp, enable the mode if ARG is omitted or
  nil, toggle it if ARG is ‘toggle’, and disable it if ARG is
  any other non-nil value.

(And place the Lisp description in a separate paragraph
from the interactive description.)

> I think minor modes should simply not allow anything but
> integers and 'toggle for ARG, avoiding this confusion.

Why?  There is no confusion possible, once the doc string 
explicitly speaks about all possible argument values.

Why would you change the behavior, instead of just clarifying
the doc?

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Noam Postavsky]

On Sun, Oct 16, 2016 at 12:12 PM, Drew Adams <drew.adams@oracle.com> wrote:
>> (define-minor-mode foo-mode nil)
>> its docstring will be
>>
>> "Toggle Foo mode on or off.
>> With a prefix argument ARG, enable Foo mode if ARG is
>> positive, and disable it otherwise.  If called from Lisp, enable
>> the mode if ARG is omitted or nil, and toggle it if ARG is ‘toggle’."
>>
>> This appears to indicate that (foo-mode 'banana)
>> should disable foo-mode, but it enables it.
>
> No, it does not suggest that.  But to be clearer, it should
> probably explicitly address the non-nil and non-`toggle' case,
> like so:
>
>   If called from Lisp, enable the mode if ARG is omitted or
>   nil, toggle it if ARG is ‘toggle’, and disable it if ARG is
>   any other non-nil value.

If the Lisp description is meant to be exhaustive by itself, we should
cover the numeric case too:

   If called from Lisp, enable the mode if ARG is omitted, nil, or a
   positive number; toggle it if ARG is `toggle'; and disable it
   otherwise.

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Drew Adams]

> >> (define-minor-mode foo-mode nil)
> >> its docstring will be
> >>
> >> "Toggle Foo mode on or off.
> >> With a prefix argument ARG, enable Foo mode if ARG is
> >> positive, and disable it otherwise.  If called from Lisp, enable
> >> the mode if ARG is omitted or nil, and toggle it if ARG is ‘toggle’."
> >>
> >> This appears to indicate that (foo-mode 'banana)
> >> should disable foo-mode, but it enables it.
> >
> > No, it does not suggest that.  But to be clearer, it should
> > probably explicitly address the non-nil and non-`toggle' case,
> > like so:
> >
> >   If called from Lisp, enable the mode if ARG is omitted or
> >   nil, toggle it if ARG is ‘toggle’, and disable it if ARG is
> >   any other non-nil value.
> 
> If the Lisp description is meant to be exhaustive by itself, we should
> cover the numeric case too:
> 
>    If called from Lisp, enable the mode if ARG is omitted, nil, or a
>    positive number; toggle it if ARG is `toggle'; and disable it
>    otherwise.

Yes, and anyway what I wrote was wrong.

The doc string of `define-minor-mode' has it right, as does your
suggestion:

  When called from Lisp, the mode command toggles the mode if the
  argument is `toggle', disables the mode if the argument is a
  non-positive integer, and enables the mode otherwise (including
  if the argument is omitted or nil or a positive integer).

I'd suggest the same order as in the `define-minor-mode' doc:

  When called from Lisp:
  * Toggle the mode if ARG is `toggle'.
  * Disable it if ARG is a non-positive integer.
  * Enable it if ARG is anything else.
  

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sun, 16 Oct 2016 12:31:51 -0400
> Cc: 24706@debbugs.gnu.org, Philipp Stephani <p.stephani2@gmail.com>
> 
> > No, it does not suggest that.  But to be clearer, it should
> > probably explicitly address the non-nil and non-`toggle' case,
> > like so:
> >
> >   If called from Lisp, enable the mode if ARG is omitted or
> >   nil, toggle it if ARG is ‘toggle’, and disable it if ARG is
> >   any other non-nil value.
> 
> If the Lisp description is meant to be exhaustive by itself, we should
> cover the numeric case too:
> 
>    If called from Lisp, enable the mode if ARG is omitted, nil, or a
>    positive number; toggle it if ARG is `toggle'; and disable it
>    otherwise.

There's a magic word 'also", which should allow us to avoid
repetitions.

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Philipp Stephani]

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

Drew Adams <drew.adams@oracle.com> schrieb am So., 16. Okt. 2016 um
18:12 Uhr:

> > (define-minor-mode foo-mode nil)
> > its docstring will be
> >
> > "Toggle Foo mode on or off.
> > With a prefix argument ARG, enable Foo mode if ARG is
> > positive, and disable it otherwise.  If called from Lisp, enable
> > the mode if ARG is omitted or nil, and toggle it if ARG is ‘toggle’."
> >
> > This appears to indicate that (foo-mode 'banana)
> > should disable foo-mode, but it enables it.
>
> No, it does not suggest that.  But to be clearer, it should
> probably explicitly address the non-nil and non-`toggle' case,
> like so:
>
>   If called from Lisp, enable the mode if ARG is omitted or
>   nil, toggle it if ARG is ‘toggle’, and disable it if ARG is
>   any other non-nil value.
>
> (And place the Lisp description in a separate paragraph
> from the interactive description.)
>
> > I think minor modes should simply not allow anything but
> > integers and 'toggle for ARG, avoiding this confusion.
>
> Why?  There is no confusion possible, once the doc string
> explicitly speaks about all possible argument values.
>
> Why would you change the behavior, instead of just clarifying
> the doc?
>

I generally prefer the behavior to be as strict as possible. Consider
(foo-mode 'disable)
If you read such code, do you assume that this enables foo-mode?
However, in this case I guess it's too late, and fixing the documentation
is indeed more appropriate. BTW, the Elisp manual has the same issue:

     The mode command should accept one optional argument.  If called
     interactively with no prefix argument, it should toggle the mode
     (i.e., enable if it is disabled, and disable if it is enabled).  If
     called interactively with a prefix argument, it should enable the
     mode if the argument is positive and disable it otherwise.

     If the mode command is called from Lisp (i.e., non-interactively),
     it should enable the mode if the argument is omitted or ‘nil’; it
     should toggle the mode if the argument is the symbol ‘toggle’;
     otherwise it should treat the argument in the same way as for an
     interactive call with a numeric prefix argument, as described
     above.

Probably this should be reworded so that the Lisp case doesn't refer to the
interactive case at all. Making the documentation obvious is more important
than avoiding repetition.

[-- Attachment #2: Type: text/html, Size: 3623 bytes --]

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 2667 bytes --]

Philipp Stephani <p.stephani2@gmail.com> schrieb am So., 16. Okt. 2016 um
20:13 Uhr:

> Drew Adams <drew.adams@oracle.com> schrieb am So., 16. Okt. 2016 um
> 18:12 Uhr:
>
> > (define-minor-mode foo-mode nil)
> > its docstring will be
> >
> > "Toggle Foo mode on or off.
> > With a prefix argument ARG, enable Foo mode if ARG is
> > positive, and disable it otherwise.  If called from Lisp, enable
> > the mode if ARG is omitted or nil, and toggle it if ARG is ‘toggle’."
> >
> > This appears to indicate that (foo-mode 'banana)
> > should disable foo-mode, but it enables it.
>
> No, it does not suggest that.  But to be clearer, it should
> probably explicitly address the non-nil and non-`toggle' case,
> like so:
>
>   If called from Lisp, enable the mode if ARG is omitted or
>   nil, toggle it if ARG is ‘toggle’, and disable it if ARG is
>   any other non-nil value.
>
> (And place the Lisp description in a separate paragraph
> from the interactive description.)
>
> > I think minor modes should simply not allow anything but
> > integers and 'toggle for ARG, avoiding this confusion.
>
> Why?  There is no confusion possible, once the doc string
> explicitly speaks about all possible argument values.
>
> Why would you change the behavior, instead of just clarifying
> the doc?
>
>
> I generally prefer the behavior to be as strict as possible. Consider
> (foo-mode 'disable)
> If you read such code, do you assume that this enables foo-mode?
> However, in this case I guess it's too late, and fixing the documentation
> is indeed more appropriate. BTW, the Elisp manual has the same issue:
>
>      The mode command should accept one optional argument.  If called
>      interactively with no prefix argument, it should toggle the mode
>      (i.e., enable if it is disabled, and disable if it is enabled).  If
>      called interactively with a prefix argument, it should enable the
>      mode if the argument is positive and disable it otherwise.
>
>      If the mode command is called from Lisp (i.e., non-interactively),
>      it should enable the mode if the argument is omitted or ‘nil’; it
>      should toggle the mode if the argument is the symbol ‘toggle’;
>      otherwise it should treat the argument in the same way as for an
>      interactive call with a numeric prefix argument, as described
>      above.
>
> Probably this should be reworded so that the Lisp case doesn't refer to
> the interactive case at all. Making the documentation obvious is more
> important than avoiding repetition.
>

Attached a patch that uses the wording from `define-minor-mode'.

[-- Attachment #1.2: Type: text/html, Size: 4683 bytes --]

[-- Attachment #2: 0001-Clarify-the-behavior-of-minor-mode-commands.txt --]
[-- Type: text/plain, Size: 5206 bytes --]

From acd639e5f31052b64a4bf1d062461e54c0c6e646 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Sun, 16 Oct 2016 20:22:24 +0200
Subject: [PATCH] Clarify the behavior of minor mode commands

See Bug#24706.

* doc/lispref/modes.texi (Minor Mode Conventions): Clarify behavior when
the argument to a minor mode command is not an integer.
* lisp/emacs-lisp/easy-mmode.el (define-minor-mode): Clarify behavior
when ARG is not an integer.
* test/lisp/emacs-lisp/easy-mmode-tests.el: New file with unit tests.
---
 doc/lispref/modes.texi                   |  8 ++--
 lisp/emacs-lisp/easy-mmode.el            |  8 +++-
 test/lisp/emacs-lisp/easy-mmode-tests.el | 69 ++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 6 deletions(-)
 create mode 100644 test/lisp/emacs-lisp/easy-mmode-tests.el

diff --git a/doc/lispref/modes.texi b/doc/lispref/modes.texi
index 368d882..ac0e95e 100644
--- a/doc/lispref/modes.texi
+++ b/doc/lispref/modes.texi
@@ -1368,10 +1368,10 @@ Minor Mode Conventions
 if the argument is positive and disable it otherwise.
 
 If the mode command is called from Lisp (i.e., non-interactively), it
-should enable the mode if the argument is omitted or @code{nil}; it
-should toggle the mode if the argument is the symbol @code{toggle};
-otherwise it should treat the argument in the same way as for an
-interactive call with a numeric prefix argument, as described above.
+should toggle the mode if the argument is the symbol @code{toggle}; it
+should disable the mode if the argument is a non-positive integer;
+otherwise, e.g., if the argument is omitted or nil or a positive
+integer, it should enable the mode.
 
 The following example shows how to implement this behavior (it is
 similar to the code generated by the @code{define-minor-mode} macro):
diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 38295c3..64ef114 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -273,8 +273,12 @@ define-minor-mode
 	 ,(or doc
 	      (format (concat "Toggle %s on or off.
 With a prefix argument ARG, enable %s if ARG is
-positive, and disable it otherwise.  If called from Lisp, enable
-the mode if ARG is omitted or nil, and toggle it if ARG is `toggle'.
+positive, and disable it otherwise.
+
+When called from Lisp, toggle the mode if ARG is `toggle',
+disable the mode if ARG is a non-positive integer, and enable the
+mode otherwise (including if ARG is omitted or nil or a positive
+integer).
 \\{%s}") pretty-name pretty-name keymap-sym))
 	 ;; Use `toggle' rather than (if ,mode 0 1) so that using
 	 ;; repeat-command still does the toggling correctly.
diff --git a/test/lisp/emacs-lisp/easy-mmode-tests.el b/test/lisp/emacs-lisp/easy-mmode-tests.el
new file mode 100644
index 0000000..2593462
--- /dev/null
+++ b/test/lisp/emacs-lisp/easy-mmode-tests.el
@@ -0,0 +1,69 @@
+;;; easy-mmode-tests.el --- tests for easy-mmode.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2016  Free Software Foundation, Inc.
+
+;; Author: Philipp Stephani <phst@google.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Unit tests for lisp/emacs-lisp/easy-mmode.el.
+
+;;; Code:
+
+(define-minor-mode easy-mmode-tests--mode nil)
+
+(ert-deftest easy-mmode-tests--modefun-nil ()
+  (let (easy-mmode-tests--mode)
+    (easy-mmode-tests--mode)
+    (should easy-mmode-tests--mode)))
+
+(ert-deftest easy-mmode-tests--modefun-0 ()
+  (let ((easy-mmode-tests--mode nil))
+    (easy-mmode-tests--mode)
+    (easy-mmode-tests--mode 0)
+    (should-not easy-mmode-tests--mode)))
+
+(ert-deftest easy-mmode-tests--modefun-+1 ()
+  (let ((easy-mmode-tests--mode nil))
+    (easy-mmode-tests--mode 1)
+    (should easy-mmode-tests--mode)))
+
+(ert-deftest easy-mmode-tests--modefun--1 ()
+  (let ((easy-mmode-tests--mode nil))
+    (easy-mmode-tests--mode)
+    (easy-mmode-tests--mode -1)
+    (should-not easy-mmode-tests--mode)))
+
+(ert-deftest easy-mmode-tests--modefun-toggle ()
+  (let ((easy-mmode-tests--mode nil))
+    (easy-mmode-tests--mode 'toggle)
+    (should easy-mmode-tests--mode)
+    (easy-mmode-tests--mode 'toggle)
+    (should-not easy-mmode-tests--mode)))
+
+(ert-deftest easy-mmode-tests--modefun-off ()
+  (let ((easy-mmode-tests--mode nil))
+    (easy-mmode-tests--mode 'off)
+    (should easy-mmode-tests--mode)))
+
+(ert-deftest easy-mmode-tests--modefun-t ()
+  (let ((easy-mmode-tests--mode nil))
+    (easy-mmode-tests--mode t)
+    (should easy-mmode-tests--mode)))
+
+;;; easy-mmode-tests.el ends here
-- 
2.10.1

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 16 Oct 2016 18:25:08 +0000
> 
> Attached a patch that uses the wording from `define-minor-mode'. 

The patch for the ELisp manual simply rearranges the same words, so
it's not clear to me why we would prefer it to what's already there.

As for the doc string, please avoid repetition, it's confusing.  I
suggested to describe the additional features when the mode is called
from Lisp by using the word "also".

Thanks.

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am So., 16. Okt. 2016 um 20:51 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 16 Oct 2016 18:25:08 +0000
> >
> > Attached a patch that uses the wording from `define-minor-mode'.
>
> The patch for the ELisp manual simply rearranges the same words, so
> it's not clear to me why we would prefer it to what's already there.
>
> As for the doc string, please avoid repetition, it's confusing.  I
> suggested to describe the additional features when the mode is called
> from Lisp by using the word "also".
>
>
I don't think there's any repetition or rearranging here. The key
difference is that when called from Lisp with an argument that is neither
nil nor an integer, the mode is also enabled. That isn't possible for
interactive calls because the argument is always a number.
Maybe something like

"With a prefix argument ARG, enable the mode if ARG is positive, and
disable it if ARG is negative or zero. Additionally, when called from Lisp,
toggle the mode if ARG is the symbol `toggle' and interpret ARG as defined
by `prefix-numeric-value' otherwise."

(and then document the behavior of prefix-numeric-value)

[-- Attachment #2: Type: text/html, Size: 1668 bytes --]

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 23 Apr 2017 17:51:32 +0000
> Cc: drew.adams@oracle.com, 24706@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> schrieb am So., 16. Okt. 2016 um 20:51 Uhr:
> 
>  > From: Philipp Stephani <p.stephani2@gmail.com>
>  > Date: Sun, 16 Oct 2016 18:25:08 +0000
>  >
>  > Attached a patch that uses the wording from `define-minor-mode'.
> 
>  The patch for the ELisp manual simply rearranges the same words, so
>  it's not clear to me why we would prefer it to what's already there.
> 
> I don't think there's any repetition or rearranging here.

Here's the ELisp manual part of your proposed change:

   If the mode command is called from Lisp (i.e., non-interactively), it
  -should enable the mode if the argument is omitted or @code{nil}; it
  -should toggle the mode if the argument is the symbol @code{toggle};
  -otherwise it should treat the argument in the same way as for an
  -interactive call with a numeric prefix argument, as described above.
  +should toggle the mode if the argument is the symbol @code{toggle}; it
  +should disable the mode if the argument is a non-positive integer;
  +otherwise, e.g., if the argument is omitted or nil or a positive
  +integer, it should enable the mode.

Don't you agree that it does little apart of re-shuffling the same
words?

> The key difference is that when called from Lisp with an
> argument that is neither nil nor an integer, the mode is also enabled.

Why would we want to require that?  This subsection describes the
conventions, it doesn't describe the effect of certain popular
implementation of those conventions, because we don't really want to
_require_ modes to behave in any way beyond the described behavior.

> "With a prefix argument ARG, enable the mode if ARG is positive, and disable it if ARG is negative or zero.

This is almost exactly the same as the current:

  With a prefix argument ARG, enable %s if ARG is
  positive, and disable it otherwise.

> Additionally, when called from Lisp, toggle the mode if ARG is the symbol `toggle' and interpret ARG as
> defined by `prefix-numeric-value' otherwise."

And this is exactly what I suggested back then:

>  As for the doc string, please avoid repetition, it's confusing. I
>  suggested to describe the additional features when the mode is called
>  from Lisp by using the word "also".

The wording I had in mind was similar to yours:

  When called from Lisp, also enable the mode if ARG is omitted or
  nil, and toggle it if ARG is `toggle'.

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Philipp Stephani]

Am Mi., 26. Apr. 2017 um 13:27 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 23 Apr 2017 17:51:32 +0000
> > Cc: drew.adams@oracle.com, 24706@debbugs.gnu.org
> >
> > Eli Zaretskii <eliz@gnu.org> schrieb am So., 16. Okt. 2016 um 20:51 Uhr:
> >
> >  > From: Philipp Stephani <p.stephani2@gmail.com>
> >  > Date: Sun, 16 Oct 2016 18:25:08 +0000
> >  >
> >  > Attached a patch that uses the wording from `define-minor-mode'.
> >
> >  The patch for the ELisp manual simply rearranges the same words, so
> >  it's not clear to me why we would prefer it to what's already there.
> >
> > I don't think there's any repetition or rearranging here.
>
> Here's the ELisp manual part of your proposed change:
>
>    If the mode command is called from Lisp (i.e., non-interactively), it
>   -should enable the mode if the argument is omitted or @code{nil}; it
>   -should toggle the mode if the argument is the symbol @code{toggle};
>   -otherwise it should treat the argument in the same way as for an
>   -interactive call with a numeric prefix argument, as described above.
>   +should toggle the mode if the argument is the symbol @code{toggle}; it
>   +should disable the mode if the argument is a non-positive integer;
>   +otherwise, e.g., if the argument is omitted or nil or a positive
>   +integer, it should enable the mode.
>
> Don't you agree that it does little apart of re-shuffling the same
> words?

It also describes what happens when the argument is neither nil nor
`toggle' nor an integer. That is currently unspecified, or rather
implicitly specified by the observable (but unspecified) behavior of
`prefix-numeric-value'.

>
> > The key difference is that when called from Lisp with an
> > argument that is neither nil nor an integer, the mode is also enabled.
>
> Why would we want to require that?  This subsection describes the
> conventions, it doesn't describe the effect of certain popular
> implementation of those conventions, because we don't really want to
> _require_ modes to behave in any way beyond the described behavior.

This isn't about the implementation but the interface, i.e. the
observable behavior of minor mode functions.

>
> > "With a prefix argument ARG, enable the mode if ARG is positive, and disable it if ARG is negative or zero.
>
> This is almost exactly the same as the current:
>
>   With a prefix argument ARG, enable %s if ARG is
>   positive, and disable it otherwise.
>
> > Additionally, when called from Lisp, toggle the mode if ARG is the symbol `toggle' and interpret ARG as
> > defined by `prefix-numeric-value' otherwise."
>
> And this is exactly what I suggested back then:
>
> >  As for the doc string, please avoid repetition, it's confusing. I
> >  suggested to describe the additional features when the mode is called
> >  from Lisp by using the word "also".
>
> The wording I had in mind was similar to yours:
>
>   When called from Lisp, also enable the mode if ARG is omitted or
>   nil, and toggle it if ARG is `toggle'.

That again doesn't describe what happens if neither of these cases apply.
(My suggestion from 2017 also wouldn't work here as-is, because the
behavior of `prefix-numeric-value' isn't defined for these cases
either.)

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 1 Aug 2020 22:47:03 +0200
> Cc: Drew Adams <drew.adams@oracle.com>, 24706@debbugs.gnu.org
> 
> Am Mi., 26. Apr. 2017 um 13:27 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > > From: Philipp Stephani <p.stephani2@gmail.com>
> > > Date: Sun, 23 Apr 2017 17:51:32 +0000
> > > Cc: drew.adams@oracle.com, 24706@debbugs.gnu.org
> > >
> > > Eli Zaretskii <eliz@gnu.org> schrieb am So., 16. Okt. 2016 um 20:51 Uhr:
> > >
> > >  > From: Philipp Stephani <p.stephani2@gmail.com>
> > >  > Date: Sun, 16 Oct 2016 18:25:08 +0000

It's hard to have a useful discussion when messages are several months
apart.

> > >  The patch for the ELisp manual simply rearranges the same words, so
> > >  it's not clear to me why we would prefer it to what's already there.
> > >
> > > I don't think there's any repetition or rearranging here.
> >
> > Here's the ELisp manual part of your proposed change:
> >
> >    If the mode command is called from Lisp (i.e., non-interactively), it
> >   -should enable the mode if the argument is omitted or @code{nil}; it
> >   -should toggle the mode if the argument is the symbol @code{toggle};
> >   -otherwise it should treat the argument in the same way as for an
> >   -interactive call with a numeric prefix argument, as described above.
> >   +should toggle the mode if the argument is the symbol @code{toggle}; it
> >   +should disable the mode if the argument is a non-positive integer;
> >   +otherwise, e.g., if the argument is omitted or nil or a positive
> >   +integer, it should enable the mode.
> >
> > Don't you agree that it does little apart of re-shuffling the same
> > words?
> 
> It also describes what happens when the argument is neither nil nor
> `toggle' nor an integer. That is currently unspecified, or rather
> implicitly specified by the observable (but unspecified) behavior of
> `prefix-numeric-value'.

I think the "otherwise" part describes that.

> > > The key difference is that when called from Lisp with an
> > > argument that is neither nil nor an integer, the mode is also enabled.
> >
> > Why would we want to require that?  This subsection describes the
> > conventions, it doesn't describe the effect of certain popular
> > implementation of those conventions, because we don't really want to
> > _require_ modes to behave in any way beyond the described behavior.
> 
> This isn't about the implementation but the interface, i.e. the
> observable behavior of minor mode functions.

I don't see how making this the matter of interface changes anything.
We still don't want to require modes to interpret the interface the
way you'd like to see.

Bottom line: I think there's nothing important left to discuss here.

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philipp Stephani <p.stephani2@gmail.com>
>> Date: Sat, 1 Aug 2020 22:47:03 +0200
>> Cc: Drew Adams <drew.adams@oracle.com>, 24706@debbugs.gnu.org
>>
>> Am Mi., 26. Apr. 2017 um 13:27 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>> >
>> > > From: Philipp Stephani <p.stephani2@gmail.com>
>> > > Date: Sun, 23 Apr 2017 17:51:32 +0000
>> > > Cc: drew.adams@oracle.com, 24706@debbugs.gnu.org
>> > >
>> > > Eli Zaretskii <eliz@gnu.org> schrieb am So., 16. Okt. 2016 um 20:51 Uhr:
>> > >
>> > >  > From: Philipp Stephani <p.stephani2@gmail.com>
>> > >  > Date: Sun, 16 Oct 2016 18:25:08 +0000
>
> It's hard to have a useful discussion when messages are several months
> apart.
>
>> > >  The patch for the ELisp manual simply rearranges the same words, so
>> > >  it's not clear to me why we would prefer it to what's already there.
>> > >
>> > > I don't think there's any repetition or rearranging here.
>> >
>> > Here's the ELisp manual part of your proposed change:
>> >
>> >    If the mode command is called from Lisp (i.e., non-interactively), it
>> >   -should enable the mode if the argument is omitted or @code{nil}; it
>> >   -should toggle the mode if the argument is the symbol @code{toggle};
>> >   -otherwise it should treat the argument in the same way as for an
>> >   -interactive call with a numeric prefix argument, as described above.
>> >   +should toggle the mode if the argument is the symbol @code{toggle}; it
>> >   +should disable the mode if the argument is a non-positive integer;
>> >   +otherwise, e.g., if the argument is omitted or nil or a positive
>> >   +integer, it should enable the mode.
>> >
>> > Don't you agree that it does little apart of re-shuffling the same
>> > words?
>>
>> It also describes what happens when the argument is neither nil nor
>> `toggle' nor an integer. That is currently unspecified, or rather
>> implicitly specified by the observable (but unspecified) behavior of
>> `prefix-numeric-value'.
>
> I think the "otherwise" part describes that.
>
>> > > The key difference is that when called from Lisp with an
>> > > argument that is neither nil nor an integer, the mode is also enabled.
>> >
>> > Why would we want to require that?  This subsection describes the
>> > conventions, it doesn't describe the effect of certain popular
>> > implementation of those conventions, because we don't really want to
>> > _require_ modes to behave in any way beyond the described behavior.
>>
>> This isn't about the implementation but the interface, i.e. the
>> observable behavior of minor mode functions.
>
> I don't see how making this the matter of interface changes anything.
> We still don't want to require modes to interpret the interface the
> way you'd like to see.
>
> Bottom line: I think there's nothing important left to discuss here.

That was 8 weeks ago, and there has been no further comments.  I'm
therefore closing this bug now.

If there is anything more to do here, please reply to this email (use
"Reply to all" in your email client) and we can reopen the bug report.

Best regards,
Stefan Kangas

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 16 Oct 2016 18:13:01 +0000
> 
>  Why would you change the behavior, instead of just clarifying
>  the doc?
> 
> I generally prefer the behavior to be as strict as possible.

Let's not consider changing long-established behavior for purely
theoretical or philosophical reasons.

> Consider
> (foo-mode 'disable)
> If you read such code, do you assume that this enables foo-mode?

It would be a silly thing to do, so it would be worth a bug report.
However, I don't really believe someone in their right minds would do
something like that.

> However, in this case I guess it's too late, and fixing the documentation is indeed more appropriate.

Exactly.

> Probably this should be reworded so that the Lisp case doesn't refer to the interactive case at all. Making the
> documentation obvious is more important than avoiding repetition.

Sorry, I disagree. with the last sentence.  Repetition is a source for
confusion.

bug#24706: 26.0.50; Minor mode functions should do strict argument type checking

[Drew Adams]

>> Why would you change the behavior, instead of just clarifying
>> the doc?
>
> I generally prefer the behavior to be as strict as possible.
> Consider (foo-mode 'disable)  If you read such code, do you
> assume that this enables foo-mode?

If I understand the function, then yes.  But I agree that the
doc could make the function behavior clearer.

That example is akin to this:

(deconst two 42)
(+ 3 two)

There are plenty of ways to write code that is perverse in
terms of readability (whether or not it has the right behavior).

> However, in this case I guess it's too late, and fixing the
> documentation is indeed more appropriate.

It would be a backward-incompatible change.  (And I do not
see any advantage to it.  But others might disagree with
me and agree with you.)