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.

diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 01f2c0d95f7130cf07726be06ff9ba27a59a0d52..f8df223ce96b650a23f979200397fc775d97eb67 100644 (file)
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -2475,6 +2475,31 @@ Argument END is the maximum bounds to search in."
          (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 @@ Argument TYPE specifies the type of question, such as `error' or `y-or-n-p'."
          "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 a4b252031fe84ff4db2ba34e6e11fd8982693ff8..3eb7da3d4a719b775f9373c332f92245356cd3e8 100644 (file)
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -146,4 +146,34 @@ See the comments in Bug#24998."
     (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