bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Stefan Kangas <stefan@marxist.se>]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> Alternative C will cause some code-churn, but is also formally the most
>> correct one. It might also save someones sanity if they are trying to
>> fit a line into a 80 or 100 character limit.
>>
>> My preference is for, in order: C > B > A.
>
> I think C is too ambitious -- there's code out-of-tree that needs to
> work in a wide range of Emacs versions, so they can't remove the
> trailing space.  So I like B (i.e.., check that there's "\\? *" at the
> end).

Sounds good.  How about the attached patch?

[-- Attachment #2: 0001-checkdoc-y-or-n-p-no-longer-needs-space.patch --]
[-- Type: text/x-diff, Size: 6710 bytes --]

From 60d56c427511a5946f823c4249d9af3421288844 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Thu, 16 Sep 2021 19:13:56 +0200
Subject: [PATCH] checkdoc: 'y-or-n-p' no longer needs space

* lisp/emacs-lisp/checkdoc.el (checkdoc-message-text-engine):
Change 'y-or-n-p' check to accept prompt ending with both "? " or "?",
that is, it no longer needs the space.  (Bug#50621)
(checkdoc--fix-y-or-n-p): New helper function.
* test/lisp/emacs-lisp/checkdoc-tests.el (checkdoc-tests-fix-y-or-n-p)
(checkdoc-tests-fix-y-or-n-p/no-change)
(checkdoc-tests-fix-y-or-n-p/with-space): New tests.
---
 lisp/emacs-lisp/checkdoc.el            | 82 ++++++++++----------------
 test/lisp/emacs-lisp/checkdoc-tests.el | 30 ++++++++++
 2 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 01f2c0d95f..f8df223ce9 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -2475,6 +2475,31 @@ checkdoc-message-text-next-string
 	  (setq return type))))
     return))
 
+(defun checkdoc--fix-y-or-n-p ()
+  "Fix `y-or-n-p' prompt to end with \"?\" or \"? \".
+The space is technically redundant, but also more compatible with
+Emacs versions before Emacs 24.1.  In the future, we might treat
+a space as a style error."
+  (when (and (save-excursion (forward-sexp 1)
+                             (forward-char -3)
+                             (not (looking-at "\\? ")))
+             (save-excursion (forward-sexp 1)
+                             (forward-char -2)
+                             (not (looking-at "\\?"))))
+    (if (and
+         (save-excursion (forward-sexp 1)
+                         (forward-char -1)
+                         (looking-at "\""))
+         (checkdoc-autofix-ask-replace
+          (match-beginning 0) (match-end 0)
+          (format-message
+           "`y-or-n-p' argument should end with \"? \".  Fix?")
+          "?\"" t))
+        nil
+      (checkdoc-create-error
+       "`y-or-n-p' argument should end with \"?\""
+       (match-beginning 0) (match-end 0)))))
+
 (defun checkdoc-message-text-engine (&optional type)
   "Return or fix errors found in strings passed to a message display function.
 According to the documentation for the function `error', the error list
@@ -2530,63 +2555,20 @@ checkdoc-message-text-engine
 	  "Error messages should *not* end with a period"
 	  (match-beginning 0) (match-end 0))
        nil)
-     ;; `y-or-n-p' documentation explicitly says:
-     ;; It should end in a space; `y-or-n-p' adds `(y or n) ' to it.
-     ;; I added the ? requirement.  Without it, it is unclear that we
-     ;; ask a question and it appears to be an undocumented style.
-     (if (eq type 'y-or-n-p)
-	 (if (not (save-excursion (forward-sexp 1)
-				  (forward-char -3)
-				  (not (looking-at "\\? "))))
-	     nil
-	   (if (save-excursion (forward-sexp 1)
-			       (forward-char -2)
-			       (looking-at "\\?"))
-	       ;; If we see a ?, then replace with "? ".
-	       (if (checkdoc-autofix-ask-replace
-		    (match-beginning 0) (match-end 0)
-                    (format-message
-                     "`y-or-n-p' argument should end with \"? \".  Fix? ")
-		    "? " t)
-		   nil
-		 (checkdoc-create-error
-		  "`y-or-n-p' argument should end with \"? \""
-		  (match-beginning 0) (match-end 0)))
-	     (if (save-excursion (forward-sexp 1)
-				 (forward-char -2)
-				 (looking-at " "))
-		 (if (checkdoc-autofix-ask-replace
-		      (match-beginning 0) (match-end 0)
-                      (format-message
-                       "`y-or-n-p' argument should end with \"? \".  Fix? ")
-		      "? " t)
-		     nil
-		   (checkdoc-create-error
-		    "`y-or-n-p' argument should end with \"? \""
-		    (match-beginning 0) (match-end 0)))
-	       (if (and ;; if this isn't true, we have a problem.
-		    (save-excursion (forward-sexp 1)
-				    (forward-char -1)
-				    (looking-at "\""))
-		    (checkdoc-autofix-ask-replace
-		     (match-beginning 0) (match-end 0)
-                     (format-message
-                      "`y-or-n-p' argument should end with \"? \".  Fix? ")
-		     "? \"" t))
-		   nil
-		 (checkdoc-create-error
-		  "`y-or-n-p' argument should end with \"? \""
-		  (match-beginning 0) (match-end 0)))))))
+     ;; From `(elisp) Programming Tips': "A question asked in the
+     ;; minibuffer with `yes-or-no-p' or `y-or-n-p' should start with
+     ;; a capital letter and end with '?'."
+     (when (eq type 'y-or-n-p)
+       (checkdoc--fix-y-or-n-p))
      ;; Now, let's just run the spell checker on this guy.
      (checkdoc-ispell-docstring-engine (save-excursion (forward-sexp 1)
-						       (point)))
-     )))
+                                                       (point))))))
 
 ;;; Auto-fix helper functions
 ;;
 (defun checkdoc-y-or-n-p (question)
   "Like `y-or-n-p', but pays attention to `checkdoc-autofix-flag'.
-Argument QUESTION is the prompt passed to `y-or-n-p'."
+  Argument QUESTION is the prompt passed to `y-or-n-p'."
   (prog1
       (if (or (not checkdoc-autofix-flag)
 	      (eq checkdoc-autofix-flag 'never))
diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
index a4b252031f..3eb7da3d4a 100644
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -146,4 +146,34 @@ checkdoc-tests-in-abbrevation-p/with-escaped-parens
     (re-search-forward "e.g")
     (should (checkdoc-in-abbreviation-p (point)))))
 
+(ert-deftest checkdoc-tests-fix-y-or-n-p ()
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (let ((standard-output (current-buffer))
+          (checkdoc-autofix-flag 'automatic))
+      (prin1 '(y-or-n-p "foo"))         ; "foo"
+      (goto-char (length "(y-or-n-p "))
+      (checkdoc--fix-y-or-n-p)
+      (should (equal (buffer-string) "(y-or-n-p \"foo?\")")))))
+
+(ert-deftest checkdoc-tests-fix-y-or-n-p/no-change ()
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (let ((standard-output (current-buffer))
+          (checkdoc-autofix-flag 'automatic))
+      (prin1 '(y-or-n-p "foo?"))        ; "foo?"
+      (goto-char (length "(y-or-n-p "))
+      (checkdoc--fix-y-or-n-p)
+      (should (equal (buffer-string) "(y-or-n-p \"foo?\")")))))
+
+(ert-deftest checkdoc-tests-fix-y-or-n-p/with-space ()
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (let ((standard-output (current-buffer))
+          (checkdoc-autofix-flag 'automatic))
+      (prin1 '(y-or-n-p "foo? "))       ; "foo? "
+      (goto-char (length "(y-or-n-p "))
+      (checkdoc--fix-y-or-n-p)
+      (should (equal (buffer-string) "(y-or-n-p \"foo? \")")))))
+
 ;;; checkdoc-tests.el ends here
-- 
2.30.2