Thanks for the feedback! > 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. I couldn't find the lisp-data-mod because I'm developing in the Emacs 27.1 , I did have to change my configuration so now I can use the build from master. The idea of the major mode is that I want to add some interactive behavior, e.g improve the tag navigation with buttons , so a new major mode can help in that task. I did change the function from defun to cl-defun, so I can set the default value of display to true, the idea of non displaying the source code file is for testing mainly. > This would better be folded into the previous patch. Done, I think I address most of the issues, sorry for the inconvenience, I'm not use to work with mailing list development. I attach a patch with the changes. Regards On 02/04/2021 00:02, Stefan Monnier wrote: >> From f8c7ee1791ec6eee1993a9e5449112579e193415 Mon Sep 17 00:00:00 2001 >> From: Fermin >> 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 >> 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 >