Re: [PATCH] (CEDET development) Improve the bovinate output buffer

[Stefan Monnier <monnier@iro.umontreal.ca>]

> From f8c7ee1791ec6eee1993a9e5449112579e193415 Mon Sep 17 00:00:00 2001
> From: Fermin <fmfs@posteo.net>
> Date: Thu, 1 Apr 2021 21:31:56 +0200
> Subject: [PATCH] Add bovinate-mode as a major mode and improve the bovinate
>  command
>
> ---

Could you expand this commit message a bit and clarify on the first line
that this regards some part of semantic?  E.g.:

    Subject: [PATCH] semantic.el (bovinate): Prettify output

    * lisp/cedet/semantic.el (bovinate-mode): New major mode.
    (bovinate): ???.

where the `???` should say what kind of improvement is brought by the
patch (like "Give a more meaningful name to the buffer"?).

> +(define-derived-mode bovinate-mode emacs-lisp-mode "Bovinate"
> +  "Bovinate buffer major-mode, giving that the list is generated with
> +`pp-to-string', the syntax is very similar to a list of Emacs Lisp.
> +This major mode helps with the syntax highlight of some of the symbols.")

AFAICT this is better served by `lisp-data-mode` than `emacs-lisp-mode`, no?
Also, try to make the first line of docstrings be self-contained.
Maybe you could just use `lisp-data-mode` without bothering to
define a new major mode for it.

>  (defun bovinate (&optional clear)
>    "Parse the current buffer.  Show output in a temp buffer.
>  Optional argument CLEAR will clear the cache before parsing.
>  If CLEAR is negative, it will do a full reparse, and also display
>  the output buffer."
>    (interactive "P")
> -  (if clear (semantic-clear-toplevel-cache))
> -  (if (eq clear '-) (setq clear -1))
> +  (when clear (semantic-clear-toplevel-cache))
>    (let* ((start (current-time))
> -	 (out (semantic-fetch-tags)))
> -    (message "Retrieving tags took %.2f seconds."
> -	     (semantic-elapsed-time start nil))
> -    (when (or (null clear) (not (listp clear))
> -	      (and (numberp clear) (< 0 clear)))
> -      (pop-to-buffer "*Parser Output*")
> -      (require 'pp)
> +	 (tags (semantic-fetch-tags))
> +         (time-elapse (semantic-elapsed-time start nil))
> +         (bovinate-buffer
> +          (format "*Parser Output from %s buffer*" (buffer-name))))
> +    (message "Retrieving tags took %.2f seconds." time-elapse)
> +    (unless (get-buffer bovinate-buffer)
> +      (setq bovinate-buffer (get-buffer-create bovinate-buffer)))
> +    (with-current-buffer bovinate-buffer

I think you can simplify the last three lines to

       (with-current-buffer (get-buffer-create bovinate-buffer)

>        (erase-buffer)
> -      (insert (pp-to-string out))
> +      (insert (pp-to-string tags))
> +      (bovinate-mode)
>        (goto-char (point-min)))))

I don't see where the new code implements the "negative CLEAR" behavior.
Also I don't see in the new code where/when the buffer is displayed.
[ Read on before replying ;-)  ]

> From 1c1673283186780e6db007f79d4f8aa864660f79 Mon Sep 17 00:00:00 2001
> From: Fermin <fmfs@posteo.net>
> Date: Thu, 1 Apr 2021 21:53:16 +0200
> Subject: [PATCH] Add display as a optional parameter for displaying the buffer
>
> ---

Similar comment abut the commit message.

>  (define-derived-mode bovinate-mode emacs-lisp-mode "Bovinate"
>    "Bovinate buffer major-mode, giving that the list is generated with
> -`pp-to-string', the syntax is very similar to a list of Emacs Lisp.
> +`pp-to-string', the syntax is very similar to a list in Emacs Lisp.
>  This major mode helps with the syntax highlight of some of the symbols.")

This would better be folded into the previous patch.

> -(defun bovinate (&optional clear)
> -  "Parse the current buffer.  Show output in a temp buffer.
> +(defun bovinate (&optional clear display)
> +  "Parse the current buffer.  Show output in a `bovinate-mode' buffer.
>  Optional argument CLEAR will clear the cache before parsing.
> -If CLEAR is negative, it will do a full reparse, and also display
> -the output buffer."
> +If non-nil DISPLAY will display the buffer `pop-to-buffer'."
>    (interactive "P")

This means that when used interactively DISPLAY will always be nil and
hence the buffer will never be "popped".  This contradicts the
first line's "Show output in a `bovinate-mode' buffer" and I also wonder
what is the benefit (is it often useful/necessary to do
`M-x bovinate` without wanting to see the resulting tags)?

>    (when clear (semantic-clear-toplevel-cache))
>    (let* ((start (current-time))
>  	 (tags (semantic-fetch-tags))
>           (time-elapse (semantic-elapsed-time start nil))
>           (bovinate-buffer
> -          (format "*Parser Output from %s buffer*" (buffer-name))))
> +          (format "*Parser Output from %s*" (buffer-name))))

This should also be folded into the previous patch.

>      (message "Retrieving tags took %.2f seconds." time-elapse)
>      (unless (get-buffer bovinate-buffer)
>        (setq bovinate-buffer (get-buffer-create bovinate-buffer)))
> @@ -359,7 +358,8 @@ the output buffer."
>        (erase-buffer)
>        (insert (pp-to-string tags))
>        (bovinate-mode)
> -      (goto-char (point-min)))))
> +      (goto-char (point-min)))
> +    (when display (pop-to-buffer bovinate-buffer))))

Hmm... overall, I think the two patches should be squashed together.


        Stefan