bug#72574: [PATCH 2/3] Fix various warnings

bug#72574: [PATCH 2/3] Fix various warnings

[Morgan Smith]

* debbugs-browse.el:
* debbugs-compat.el:
* debbugs-gnu.el:
* debbugs-org.el:
* debbugs.el:
Fix various warnings and checkdoc warnings.
---
 debbugs-browse.el |  3 ++-
 debbugs-compat.el |  3 +++
 debbugs-gnu.el    | 22 +++++++++++-----------
 debbugs-org.el    |  7 ++++---
 debbugs.el        | 12 +++++++-----
 5 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/debbugs-browse.el b/debbugs-browse.el
index 3b439b08b5..900adec818 100644
--- a/debbugs-browse.el
+++ b/debbugs-browse.el
@@ -54,6 +54,7 @@ This can be either `debbugs-gnu-bugs' or `debbugs-org-bugs'."
 
 ;;;###autoload
 (defun debbugs-browse-url (url &optional _new-window)
+  "Browse bug at URL using debbugs."
   (when (and (stringp url)
 	     (string-match debbugs-browse-url-regexp url))
     (funcall debbugs-browse-function (string-to-number (match-string 3 url)))
@@ -67,7 +68,7 @@ This can be either `debbugs-gnu-bugs' or `debbugs-org-bugs'."
 
 ;;;###autoload
 (define-minor-mode debbugs-browse-mode
-  "Browse GNU Debbugs bug URLs with debbugs-gnu or debbugs-org.
+  "Browse GNU Debbugs bug URLs with `debbugs-gnu' or `debbugs-org'.
 With a prefix argument ARG, enable Debbugs Browse mode if ARG is
 positive, and disable it otherwise.  If called from Lisp, enable
 the mode if ARG is omitted or nil.
diff --git a/debbugs-compat.el b/debbugs-compat.el
index 29509fa6d2..96628a1921 100644
--- a/debbugs-compat.el
+++ b/debbugs-compat.el
@@ -21,6 +21,9 @@
 ;; You should have received a copy of the GNU General Public License
 ;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
 
+;;; Commentary:
+;;; Code:
+
 ;; Function `string-replace' is new in Emacs 28.1.
 (defalias 'debbugs-compat-string-replace
   (if (fboundp 'string-replace)
diff --git a/debbugs-gnu.el b/debbugs-gnu.el
index 6b6dd4992b..68599cff7f 100644
--- a/debbugs-gnu.el
+++ b/debbugs-gnu.el
@@ -33,7 +33,7 @@
 ;;
 ;;   M-x debbugs-gnu
 
-;; It asks for the severities, for which bugs shall be shown. This can
+;; It asks for the severities, for which bugs shall be shown.  This can
 ;; be either just one severity, or a list of severities, separated by
 ;; comma.  Valid severities are "serious", "important", "normal",
 ;; "minor" or "wishlist".  Severities "critical" and "grave" are not
@@ -243,7 +243,7 @@
 (defvar debbugs-gnu-limit)
 
 (defgroup debbugs-gnu ()
-  "UI for the debbugs.gnu.org bug tracker."
+  "UI for the debbugs bug tracker."
   :group 'debbugs
   :version "24.1")
 
@@ -426,7 +426,7 @@ They haven't been touched more than a week.")
 
 (defvar debbugs-gnu-persistency-file
   (expand-file-name (locate-user-emacs-file "debbugs"))
-  "File name of a persistency store for debbugs variables")
+  "File name of a persistency store for debbugs variables.")
 
 (defun debbugs-gnu-dump-persistency-file ()
   "Function to store debbugs variables persistently."
@@ -698,7 +698,7 @@ depend on PHRASE being a string, or nil.  See Info node
 
 ;;;###autoload
 (defun debbugs-gnu-package (&optional packages)
-  "List the bug reports of default packages, divided by severity."
+  "List the bug reports of PACKAGES or default packages, divided by severity."
   (interactive
      (list
       (if current-prefix-arg
@@ -2548,7 +2548,7 @@ If given a prefix, patch in the branch directory instead.
 If SELECTIVELY, query the user before applying the patch."
   (interactive "P")
   (unless (eq debbugs-gnu-mail-backend 'gnus)
-    (error "This function only works with Gnus."))
+    (error "This function only works with Gnus"))
   (add-hook 'diff-mode-hook #'debbugs-gnu-diff-mode)
   (debbugs-gnu-init-current-directory branch)
   (let ((rej (expand-file-name "debbugs-gnu.rej" temporary-file-directory))
@@ -2695,12 +2695,12 @@ If SELECTIVELY, query the user before applying the patch."
 		       nil t)))
     (forward-line 2)))
 
-(defun debbugs-gnu-find-contributor (string)
-  "Search through ChangeLogs to find contributors."
+(defun debbugs-gnu-find-contributor (contributor)
+  "Search through ChangeLogs to find CONTRIBUTOR."
   (interactive "sContributor match: ")
   (debbugs-gnu-init-current-directory)
   (let ((found 0)
-	(match (concat "^[0-9].*" string)))
+	(match (concat "^[0-9].*" contributor)))
     (dolist (file (directory-files-recursively
 		   debbugs-gnu-current-directory "ChangeLog\\(\\.[0-9]+\\)?\\'"))
       (with-temp-buffer
@@ -2710,7 +2710,7 @@ If SELECTIVELY, query the user before applying the patch."
 	(while (and (re-search-forward match nil t)
 		    (not (looking-at ".*tiny change")))
 	  (cl-incf found))))
-    (message "%s is a contributor %d times" string found)
+    (message "%s is a contributor %d times" contributor found)
     found))
 
 (defvar debbugs-gnu-patch-subject nil)
@@ -2719,7 +2719,7 @@ If SELECTIVELY, query the user before applying the patch."
   "Add a ChangeLog from a recently applied patch from a third party."
   (interactive)
   (unless (eq debbugs-gnu-mail-backend 'gnus)
-    (error "This function only works with Gnus."))
+    (error "This function only works with Gnus"))
   (let (from subject patch-subject changelog
 	     patch-from)
     (with-current-buffer gnus-article-buffer
@@ -2848,7 +2848,7 @@ If SELECTIVELY, query the user before applying the patch."
     map))
 
 (define-minor-mode debbugs-gnu-log-edit-mode
-  "Minor mode for providing a debbugs interface in log-edit buffers.
+  "Minor mode for providing a debbugs interface in `log-edit' buffers.
 
 \\{debbugs-gnu-log-edit-mode-map}"
   :lighter " Debbugs" :keymap debbugs-gnu-log-edit-mode-map)
diff --git a/debbugs-org.el b/debbugs-org.el
index c8a94c32dc..0a3a5b2a65 100644
--- a/debbugs-org.el
+++ b/debbugs-org.el
@@ -31,7 +31,7 @@
 ;;
 ;;   M-x debbugs-org
 
-;; It asks for the severities, for which bugs shall be shown. This can
+;; It asks for the severities, for which bugs shall be shown.  This can
 ;; be either just one severity, or a list of severities, separated by
 ;; comma.  Valid severities are "serious", "important", "normal",
 ;; "minor" or "wishlist".  Severities "critical" and "grave" are not
@@ -140,7 +140,7 @@
   "Mapping of debbugs severities to TODO priorities.")
 
 (defun debbugs-org-get-severity-priority (state)
-  "Returns the TODO priority of STATE."
+  "Return the TODO priority of STATE."
   (or (cdr (assoc (alist-get 'severity state) debbugs-org-severity-priority))
       (cdr (assoc "minor" debbugs-org-severity-priority))))
 
@@ -319,7 +319,7 @@ the corresponding buffer (e.g. by closing Emacs)."
 
 ;;;###autoload
 (define-minor-mode debbugs-org-mode
-  "Minor mode for providing a debbugs interface in org-mode buffers.
+  "Minor mode for providing a debbugs interface in `org-mode' buffers.
 
 \\{debbugs-org-mode-map}"
   :lighter " Debbugs" :keymap debbugs-org-mode-map
@@ -374,3 +374,4 @@ defined."
 ;; - Sort according to different TODO properties.
 
 (provide 'debbugs-org)
+;;; debbugs-org.el ends here
diff --git a/debbugs.el b/debbugs.el
index c90fb1008c..627cdf163d 100644
--- a/debbugs.el
+++ b/debbugs.el
@@ -44,7 +44,7 @@
 (eval-when-compile (require 'cl-lib))
 
 (defgroup debbugs nil
-  "Debbugs library"
+  "Debbugs library."
   :group 'hypermedia)
 
 (defcustom debbugs-servers
@@ -56,10 +56,10 @@
      :bugreport-url "https://bugs.debian.org/cgi-bin/bugreport.cgi"))
   "*List of Debbugs server specifiers.
 Each entry is a list that contains a string identifying the port
-name and the server parameters in keyword-value form. Allowed
+name and the server parameters in keyword-value form.  Allowed
 keywords are:
 
-`:wsdl' -- Location of WSDL. The value is a string with URL that
+`:wsdl' -- Location of WSDL.  The value is a string with URL that
 should return the WSDL specification of Debbugs/SOAP service.
 
 `:bugreport-url' -- URL of the server script that returns mboxes
@@ -121,7 +121,9 @@ t or 0 disables caching, nil disables expiring."
   "The object manipulated by `debbugs-soap-invoke-async'.")
 
 (defun debbugs-soap-invoke-async (operation-name &rest parameters)
-  "Invoke the SOAP connection asynchronously."
+  "Invoke the SOAP connection asynchronously.
+
+OPERATION-NAME and PARAMETERS are as described in `soap-invoke'."
   (apply
    #'soap-invoke-async
    (lambda (response &rest _args)
@@ -381,7 +383,7 @@ Every returned entry is an association list with the following attributes:
 
   `package': A list of package names the bug belongs to.
 
-  `severity': The severity of the bug report. This can be
+  `severity': The severity of the bug report.  This can be
   \"critical\", \"grave\", \"serious\", \"important\",
   \"normal\", \"minor\" or \"wishlist\".
 
-- 
2.45.2

bug#72574: [PATCH 2/3] Fix various warnings

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

> * debbugs-browse.el:
> * debbugs-compat.el:
> * debbugs-gnu.el:
> * debbugs-org.el:
> * debbugs.el:
> Fix various warnings and checkdoc warnings.
> ---
>  debbugs-browse.el |  3 ++-
>  debbugs-compat.el |  3 +++
>  debbugs-gnu.el    | 22 +++++++++++-----------
>  debbugs-org.el    |  7 ++++---
>  debbugs.el        | 12 +++++++-----
>  5 files changed, 27 insertions(+), 20 deletions(-)

Pushed.

bug#72574: [PATCH 3/3] Add tests

[Morgan Smith]

* Makefile: New file.
* test/test-debbugs.el: New file.
---
 Makefile             |   4 ++
 test/test-debbugs.el | 101 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 Makefile
 create mode 100644 test/test-debbugs.el

diff --git a/Makefile b/Makefile
new file mode 100644
index 0000000000..3a4b06a76e
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,4 @@
+.PHONY: check
+
+check:
+	emacs -Q --batch -L . -l test/* -f ert-run-tests-batch-and-exit
diff --git a/test/test-debbugs.el b/test/test-debbugs.el
new file mode 100644
index 0000000000..8eca3fe3a8
--- /dev/null
+++ b/test/test-debbugs.el
@@ -0,0 +1,101 @@
+;;; test-debbugs.el --- tests for debbugs.el -*- lexical-binding: t; -*-
+
+;;; Commentary:
+
+;; Please ensure tests don't actually make network calls.
+
+;;; Code:
+
+(require 'debbugs)
+
+;;; Helper Data:
+
+;; Generated using this:
+;; (soap-invoke debbugs-wsdl debbugs-port "get_status" [64064])
+(defvar test-bug-status-soap-return
+  '(((item (key . 64064)
+        (value (package . "emacs") (found_date) (last_modified . 1689593050)
+               (affects) (date . 1686745022) (fixed_versions)
+               (originator . "Morgan Smith <Morgan.J.Smith@outlook.com>")
+               (blocks) (archived . 1) (found) (unarchived) (tags . "patch")
+               (severity . "normal") (location . "archive") (owner) (fixed)
+               (blockedby) (pending . "done") (keywords . "patch") (id . 64064)
+               (found_versions) (mergedwith) (summary) (forwarded)
+               (log_modified . 1689593050)
+               (done . "Michael Albinus <michael.albinus@gmx.de>")
+               (source . "unknown")
+               (msgid
+                . "<DM5PR03MB31632E3A4FE170C62E7D4B0CC55AA@DM5PR03MB3163.namprd03.prod.outlook.com>")
+               (bug_num . 64064) (subject . "[PATCH 0/4] debbugs improvements")
+               (fixed_date))))))
+
+;; Generated using this:
+;; (debbugs-get-status 64064)
+(defvar test-bug-status
+  '(((cache_time . 5000) (source . "unknown") (unarchived)
+     (keywords "patch") (blocks) (pending . "done") (severity . "normal") (found)
+     (done . "Michael Albinus <michael.albinus@gmx.de>") (location . "archive")
+     (log_modified . 1689593050) (subject . "[PATCH 0/4] debbugs improvements")
+     (last_modified . 1689593050)
+     (originator . "Morgan Smith <Morgan.J.Smith@outlook.com>") (archived . t)
+     (blockedby) (affects) (mergedwith) (summary) (date . 1686745022)
+     (fixed_versions) (id . 64064) (fixed) (found_date) (forwarded) (tags "patch")
+     (msgid
+      . "<DM5PR03MB31632E3A4FE170C62E7D4B0CC55AA@DM5PR03MB3163.namprd03.prod.outlook.com>")
+     (owner) (found_versions) (fixed_date) (bug_num . 64064) (package "emacs"))))
+
+;;; Helper Functions:
+
+(defvar test-soap-operation-name nil)
+(defvar test-soap-parameters nil)
+(defun soap-invoke-internal (callback _cbargs _wsdl _service operation-name &rest parameters)
+  "Over-ride for testing"
+  (setq test-soap-operation-name operation-name)
+  (setq test-soap-parameters parameters)
+  (let ((return
+         (cond ((string-equal operation-name "get_status") test-bug-status-soap-return)
+               ((string-equal operation-name "get_usertag") '(((hi))))
+               (t '((0))))))
+    (if callback
+        (progn
+          (funcall callback return)
+          nil)
+      return)))
+
+;;; Tests:
+
+(ert-deftest test-debbugs-get-bugs ()
+  (let (test-soap-operation-name test-soap-parameters)
+    (debbugs-get-bugs
+     :tag "patch"
+     :severity "critical"
+     :status "open"
+     :status "forwarded")
+    (should (string-equal test-soap-operation-name "get_bugs"))
+    (should (equal test-soap-parameters '(["tag" "patch" "severity" "critical"
+                                           "status" "open" "status" "forwarded"])))))
+
+(ert-deftest test-debbugs-newest-bugs ()
+  (let (test-soap-operation-name test-soap-parameters)
+    (debbugs-newest-bugs 4)
+    (should (string-equal test-soap-operation-name "newest_bugs"))
+    (should (equal test-soap-parameters '(4)))))
+
+(ert-deftest test-debbugs-get-status ()
+  (let ((original-float-time (symbol-function 'float-time))
+        test-soap-operation-name test-soap-parameters)
+    (fset 'float-time (lambda (&optional _specified-time) 5000))
+    (should (= (float-time) 5000))
+    (should (equal (sort (car (debbugs-get-status 64064))) (sort (car test-bug-status))))
+    (should (string-equal test-soap-operation-name "get_status"))
+    (should (equal test-soap-parameters '([64064])))
+    (fset 'float-time original-float-time)))
+
+(ert-deftest test-debbugs-get-usertag ()
+  (let (test-soap-operation-name test-soap-parameters)
+    (should (equal (debbugs-get-usertag :user "emacs") '("hi")))
+    (should (string-equal test-soap-operation-name "get_usertag"))
+    (should (equal test-soap-parameters '("emacs")))))
+
+(provide 'test-debbugs)
+;;; test-debbugs.el ends here
-- 
2.45.2

bug#72574: [PATCH 3/3] Add tests

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

Hi Morgan,

here are my comments:

> * test/test-debbugs.el: New file.

I propose to follow Emacs naming conventions. A file PACKAGE.el has a
corresponding file PACKAGE-tests.el. So we might call the file now
debbugs-tests.el. If more test functions arrive, we might split this
file into the different debbugs-*-tests.el files.

> +++ b/Makefile
> +.PHONY: check
> +
> +check:
> +	emacs -Q --batch -L . -l test/* -f ert-run-tests-batch-and-exit

Well, you run the tests on the source file. Usually, batch tests are
run on compiled files. So you might add rules for compiling the
debbugs*.el, and the test/debbugs-tests*.el files first.

You call the program 'emacs', hardcoded. This is OK as default, but
people (me!) might want to test with different Emacs versions on the
same machine. Please add a variable 'EMACS ?= emacs', and use it.

Don't use '-l test/*', better is -l 'test/*.el'. Perhaps you want to add
something else in that directory, like a README?

> +++ b/test/test-debbugs.el
> +;;; test-debbugs.el --- tests for debbugs.el -*- lexical-binding: t; -*-

The Copyright notice and the GNU GPL text is missing. And also the
Author: and Package: header lines.

> +;; Generated using this:
> +;; (soap-invoke debbugs-wsdl debbugs-port "get_status" [64064])
> +(defvar test-bug-status-soap-return

Please prefix helper objects with 'debbugs-test--'. Yes, two hyphens,
'cos they are helpers.

It has a fixed value, so it might be a defconst.

> +;; Generated using this:
> +;; (debbugs-get-status 64064)
> +(defvar test-bug-status

Same comments as above.

> +(defvar test-soap-operation-name nil)
> +(defvar test-soap-parameters nil)

Please adapt the names.

> +(defun soap-invoke-internal (callback _cbargs _wsdl _service operation-name &rest parameters)
> +  "Over-ride for testing"
> +  (setq test-soap-operation-name operation-name)
> +  (setq test-soap-parameters parameters)
> +  (let ((return
> +         (cond ((string-equal operation-name "get_status") test-bug-status-soap-return)
> +               ((string-equal operation-name "get_usertag") '(((hi))))
> +               (t '((0))))))
> +    (if callback
> +        (progn
> +          (funcall callback return)
> +          nil)
> +      return)))

Don't override the function this way. It is better to write a helper
function debbugs-test--soap-invoke-internal, and to override the
original function with something like

--8<---------------cut here---------------start------------->8---
(add-function
 :override (symbol-function #'soap-invoke-internal)
 #'debbugs-test--soap-invoke-internal)
--8<---------------cut here---------------end--------------->8---

Another technique is to use cl-letf, see example below.

> +(ert-deftest test-debbugs-get-bugs ()

Please rename all tests to debbugs-test-*. This is not formalized in the
Emacs source tree, but good practice.

> +(ert-deftest test-debbugs-get-status ()
> +  (let ((original-float-time (symbol-function 'float-time))
> +        test-soap-operation-name test-soap-parameters)
> +    (fset 'float-time (lambda (&optional _specified-time) 5000))
> +    (should (= (float-time) 5000))
> +    (should (equal (sort (car (debbugs-get-status 64064))) (sort (car test-bug-status))))
> +    (should (string-equal test-soap-operation-name "get_status"))
> +    (should (equal test-soap-parameters '([64064])))
> +    (fset 'float-time original-float-time)))

Please respect max line width of 80.

Please use cl-letf instead of fset. Something like

--8<---------------cut here---------------start------------->8---
(ert-deftest debbugs-test-get-status ()
  (cl-letf (((symbol-function #'float-time)
	     (lambda (&optional _specified-time) 5000)))
    (let (test-soap-operation-name test-soap-parameters)
      (should (= (float-time) 5000))
      (should (equal (sort (car (debbugs-get-status 64064)))
                     (sort (car test-bug-status))))
      (should (string-equal test-soap-operation-name "get_status"))
      (should (equal test-soap-parameters '([64064]))))))
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#72574: [PATCH 3/3] Add tests

[Morgan Smith]

Hi Michael,

I really appreciate you taking the time let me know all of this.  It 
would have taken me a while to figure out all of the best practices on 
my own.  This saves me a lot of time!  I'll get you an updated patch soon.

Thank you!

Morgan