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

From: Philipp Stephani <[email-masked]>
Date: Sun, 16 Oct 2016 18:25:08 +0000

Philipp Stephani <[email-masked]> schrieb am So., 16. Okt. 2016 um
20:13 Uhr:

> Drew Adams <[email-masked]> 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'.
From acd639e5f31052b64a4bf1d062461e54c0c6e646 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <[email-masked]>
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 <[email-masked]>
+
+;; 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