Re: [NonGNU ELPA] New package: eldoc-diffstat

[Philip Kaludercic]

Johann Klähn <johann@jklaehn.de> writes:

> I would like to submit a new package to NonGNU ELPA: eldoc-diffstat.
>
>   https://github.com/kljohann/eldoc-diffstat/

I also have a few comments:

diff --git a/eldoc-diffstat.el b/eldoc-diffstat.el
index 6433e3e..5bbcd33 100644
--- a/eldoc-diffstat.el
+++ b/eldoc-diffstat.el
@@ -55,10 +55,10 @@ a property to the process object.")
 
 (defconst eldoc-diffstat--commands
   '((Git "git" "--no-pager" "show" "--color=always"
-         "--format=format:%an <%ae>, %aD:%n%s" "--stat=80")
+        "--format=format:%an <%ae>, %aD:%n%s" "--stat=80")
     (Hg "hg" "--pager=never" "log" "--color=always"
-        "--template" "{author}, {date|rfc822date}:\n{desc|firstline}\n"
-        "--stat" "--rev"))
+       "--template" "{author}, {date|rfc822date}:\n{desc|firstline}\n"
+       "--stat" "--rev"))
   "Alist mapping VCS backend to the command to use for computing the 
diffstat.")
 
 ;;;###autoload
@@ -66,7 +66,7 @@ a property to the process object.")
   "Configure eldoc buffer-locally to display diffstat for revision at point."
   (interactive)
   (add-hook 'eldoc-documentation-functions
-            #'eldoc-diffstat--docstring nil 'local)
+           #'eldoc-diffstat--docstring nil 'local)
   (unless (bound-and-true-p eldoc-mode)
     (eldoc-mode)))
 
@@ -74,11 +74,11 @@ a property to the process object.")
   "Display diffstat for revision at point by calling CALLBACK.
 Intended for `eldoc-documentation-functions'."
   (when-let* ((info (eldoc-diffstat--get-revision-info))
-              (backend (car info))
-              (revision (cdr info))
-              (command (alist-get backend eldoc-diffstat--commands)))
-    (if-let ((result (eldoc-diffstat--get-cache info)))
-        (funcall callback result)
+             (backend (car info))
+             (revision (cdr info))
+             (command (alist-get backend eldoc-diffstat--commands)))
+    (if-let* ((result (eldoc-diffstat--get-cache info)))
+       (funcall callback result)
       (eldoc-diffstat--docstring-1 (append command (list revision)) callback 
info))))
 
 (defun eldoc-diffstat--get-revision-info ()
@@ -87,34 +87,34 @@ The returned record should be a cons cell of the form 
(BACKEND . REVISION) where
 BACKEND is a symbol representing the version control system and REVISION is
 a string identifying the specific revision."
   (cond
-   ((when-let (((fboundp 'magit-stash-at-point))
-               (revision (magit-stash-at-point)))
+   ((when-let* (((fboundp 'magit-stash-at-point))
+               (revision (magit-stash-at-point)))
       (cons 'Git revision)))
-   ((when-let (((fboundp 'magit-commit-at-point))
-               (revision (magit-commit-at-point)))
+   ((when-let* (((fboundp 'magit-commit-at-point))
+               (revision (magit-commit-at-point)))
       (cons 'Git revision)))
    ((and (derived-mode-p 'git-rebase-mode)
-         (fboundp 'git-rebase-current-line)
-         (with-slots (action-type target)
-             (git-rebase-current-line)
-           (and (eq action-type 'commit)
-                (cons 'Git target)))))
+        (fboundp 'git-rebase-current-line)
+        (with-slots (action-type target)
+            (git-rebase-current-line)
+          (and (eq action-type 'commit)
+               (cons 'Git target)))))
    ((and (derived-mode-p 'vc-annotate-mode)
-         (boundp 'vc-annotate-backend)
-         (fboundp 'vc-annotate-extract-revision-at-line))
+        (boundp 'vc-annotate-backend)
+        (fboundp 'vc-annotate-extract-revision-at-line))
     (cons vc-annotate-backend
-          (car (vc-annotate-extract-revision-at-line))))
+         (car (vc-annotate-extract-revision-at-line))))
    ((and (derived-mode-p 'log-view-mode)
-         (boundp 'log-view-vc-backend))
+        (boundp 'log-view-vc-backend))
     (cons log-view-vc-backend
-          (log-view-current-tag)))))
+         (log-view-current-tag)))))
 
 (defun eldoc-diffstat--get-cache (revision-info)
   "Retrieve cached diffstat result for REVISION-INFO if available."
   (when-let* ((proc eldoc-diffstat--process)
-              ((processp proc))
-              (cached-result (process-get proc :cached-result))
-              ((equal revision-info (car cached-result))))
+             ((processp proc))
+             (cached-result (process-get proc :cached-result))
+             ((equal revision-info (car cached-result))))
     (cdr cached-result)))
 
 (defun eldoc-diffstat--docstring-1 (command callback revision-info)
@@ -126,7 +126,7 @@ caching the result, see `eldoc-diffstat--get-cache' for 
details."
   (when (processp eldoc-diffstat--process)
     (when (process-live-p eldoc-diffstat--process)
       (let (confirm-kill-processes)
-        (kill-process eldoc-diffstat--process)))
+       (kill-process eldoc-diffstat--process)))
     (kill-buffer (process-buffer eldoc-diffstat--process)))
 
   (setq
@@ -137,8 +137,9 @@ caching the result, see `eldoc-diffstat--get-cache' for 
details."
     :noquery t
     :command command
     :sentinel
-    (lambda (&rest args)
-      (apply #'eldoc-diffstat--sentinel callback args))))
+    (apply-partially #'eldoc-diffstat--sentinel callback)))
+  ;; Is it an issue that there is the slight possibility of a race
+  ;; condition here?
   (process-put eldoc-diffstat--process :revision-info revision-info)
 
   ;; Signal that the doc string is computed asynchronously.
@@ -150,41 +151,47 @@ caching the result, see `eldoc-diffstat--get-cache' for 
details."
     (with-current-buffer (process-buffer proc)
       (eldoc-diffstat--format-output-buffer)
       (let ((result (buffer-string))
-            (revision-info (process-get eldoc-diffstat--process 
:revision-info)))
-        (process-put eldoc-diffstat--process :cached-result
-                     (cons revision-info result))
-        (funcall callback result)))))
+           (revision-info (process-get eldoc-diffstat--process 
:revision-info)))
+       (process-put eldoc-diffstat--process :cached-result
+                    (cons revision-info result))
+       (funcall callback result)))))
 
 (defun eldoc-diffstat--format-output-buffer ()
-  "Format the diffstat output."
+  "Format the diffstat output in the current buffer."
   ;; Translate color control sequences into text properties.
   (let ((ansi-color-apply-face-function
-         (lambda (beg end face)
-           (put-text-property beg end 'face face))))
+        (lambda (beg end face)
+          (put-text-property beg end 'face face))))
     (ansi-color-apply-on-region (point-min) (point-max)))
 
-  ;; Delete trailing blank lines.
-  (goto-char (point-max))
-  (delete-blank-lines)
-
-  ;; Make first line bold.
   (goto-char (point-min))
-  (put-text-property (point)
-                     (line-end-position)
-                     'face 'bold)
-
-  ;; Join second line.
-  (forward-line)
-  (join-line)
-
-  ;; Move summary to the top and make it italic.
-  (forward-line)
-  (reverse-region (point) (point-max))
-  (put-text-property (point)
-                     (line-end-position)
-                     'face 'italic)
-  (forward-line)
-  (reverse-region (point) (point-max)))
+  (unless (looking-at
+          (rx buffer-start point
+              ;; Commit author and date:
+              (group (+ not-newline)) (group ?\n)
+              ;; First lie of the commit message
+              (group (+ not-newline)) (group ?\n)
+              ;; File changes
+              (group (+? anything))
+              ;; Commit Summary
+              (group (+ not-newline))
+              (* space) buffer-end))
+    ;; If this occurs, then we know that the buffer doesn't look have
+    ;; the expected textual form!
+    (error "Malformed diffstat"))
+
+  (put-text-property                   ;make first line bold
+   (match-beginning 1) (match-end 1)
+   'face 'bold)
+  (replace-match " " nil nil nil 2)    ;join the first two lines
+  (reverse-region                      ;reverse the diffstat order
+   (match-beginning 5) (match-end 5))
+  (let ((summary (delete-and-extract-region
+                 (match-beginning 6)
+                 (match-end 6))))
+    (replace-match                     ;move summary up and italicise
+     (concat "\n" (propertize summary 'face 'italic) "\n")
+     nil nil nil 4)))
 
 (provide 'eldoc-diffstat)
 ;;; eldoc-diffstat.el ends here

To summarise, when-let/if-let have been deprecated in Emacs 31 so it is
best to stick to the star'ed variants.  The rewriting of
`eldoc-diffstat--format-output-buffer' might be controversial, but I
feel that using a regular expression to destruct the buffer feels more
robust.

It would also be nice if you could add a .elpaignore file to exclude the
screenshot from being bundled with the tarball.

> It provides a way to display VCS diffstat information via eldoc.  I.e.,
> if point rests on a commit in a magit, vc-annotate, or log-view buffer,
> the Git or Mercurial diffstat summary will be shown in the echo area.
> For example (though it uses ansi-colors and fontification, so you might
> prefer to look at the screenshot in the repo):
>
>   Johann Klähn <xyz@localhost>, Thu, 3 Oct 2024 15:45:10 +0200: Minor 
> refactoring
>    1 file changed, 93 insertions(+), 62 deletions(-)
>    eldoc-diffstat.el | 155 
> ++++++++++++++++++++++++++++++++----------------------
>
> It's adapted from Tassilo Horn's 2022 blog post “Using Eldoc with Magit
> (asynchronously!)”: 
> https://www.tsdh.org/posts/2022-07-20-using-eldoc-with-magit-async.html
>
> Thanks
> Johann