[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] (CEDET development) Improve the bovinate output buffer
From: |
Stefan Monnier |
Subject: |
Re: [PATCH] (CEDET development) Improve the bovinate output buffer |
Date: |
Thu, 01 Apr 2021 18:02:48 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
> 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