On 30/12/2023 09:33, Eli Zaretskii wrote: >> Date: Sat, 30 Dec 2023 05:05:01 +0200 >> Cc: 67687@debbugs.gnu.org, eskinjp@gmail.com, stefankangas@gmail.com >> From: Dmitry Gutov >> >>>> +(defcustom etags-regen-tags-file "TAGS" >>>> + "Name of the tags file to create inside the project. >>> >>> This and the other defcustom's here should say in the first line of >>> the doc string that they are for etags-regen-mode. This will help >>> discoverability and also produce a more helpful display with the >>> various apropos commands. >> >> I've tried, but it seems hard to fit into most of them while keeping to >> the requisite max number of columns. Only managed to fit that into >> etags-regen-program and etags-regen-file-extensions. >> >> TBH, most of the time it would seem superfluous, given the namespaced >> names. But it's probably good to mention in 'etags-regen-program', on >> balance. > > I suggest some minor improvements in this area below. All right. >>>> +;;;###autoload >>>> +(put 'etags-regen-file-extensions 'safe-local-variable >>>> + (lambda (value) (and (listp value) (seq-every-p #'stringp value)))) >>> >>> Why not use list-of-strings-p here? >> >> Again, that "core ELPA" consideration. We could deploy this feature to a >> number of released Emacs versions, if we don't introduce such dependencies. > > Isn't this covered by the compat package on ELPA? If not, I think it > should be. These forms go into generated autoloads file for each installed package (*-autoloads.el). I think compat doesn't make list-of-string-p autoloaded, and autoloads files don't usually have (require ...) forms. So while I haven't really tested this and could be missing something, it seems brittle to rely on 'compat' for this function (if at all possible). >>>> + (lambda (f) (or (not (string-match-p match-re f)) >>>> + (string-match-p "/\\.#" f) >>> >>> Is that '/' there to detect regexps for absolute file names? If so, >>> that won't work for Windows. >> >> It's to detect backup files. > > Can you add a comment there to that effect? Added. >>>> +(defun etags-regen--ignore-regexp (ignore) >>>> + (require 'dired) >>>> + ;; It's somewhat brittle to rely on Dired. >>>> + (let ((re (dired-glob-regexp ignore))) >>>> + ;; We could implement root anchoring here, but \\= doesn't work in >>>> + ;; string-match :-(. >>>> + (concat (unless (eq ?/ (aref re 3)) "/") >>>> + ;; Cutting off the anchors. >>>> + (substring re 2 (- (length re) 2)) >>>> + (unless (eq ?/ (aref re (- (length re) 3))) >>>> + ;; Either match a full name segment, or eos. >>>> + "\\(?:/\\|\\'\\)")))) >>> >>> Same here: what is the purpose of comparisons with a slash? I think >>> we need some more comments there explaining the logic of the code. >> >> We compare with a slash to see whether the glob was matching against a >> directory (in which case it's already anchored to the name of a file >> name segment), otherwise we add such anchoring to either the end of a >> file name segment or eos (thus allowing a glob match both directory >> names and file names). >> >> Added a shorter comment saying the same. > > Thanks, but I miss in that comment explanations of the "magic" > constants 2 and 3. Could we add that, please? 2 is the length of both anchors, 3 is the index of the character right after the anchor. There is already a comment about cutting off the anchors, I've expanded it a bit. >>>> +(defun etags-regen--append-tags (&rest file-names) >>>> + (goto-char (point-max)) >>>> + (let ((options (etags-regen--build-program-options (etags-regen--ctags-p))) >>>> + (inhibit-read-only t)) >>>> + ;; XXX: call-process is significantly faster, though. >>>> + ;; Like 10ms vs 20ms here. >>>> + (shell-command >>>> + (format "%s %s %s -o -" >>>> + etags-regen-program (mapconcat #'identity options " ") >>>> + (mapconcat #'identity file-names " ")) >>>> + t etags-regen--errors-buffer-name)) >>> >>> Should we indeed use call-process? >> >> Something for later improvement. >> >> Looking at the code, I believe I decided to use 'shell-command' for the >> first version because of how easy it makes to output stderr to a >> separate buffer. call-process only offers writing them to a file. > > How about mentioning this issue in that comment? Added. >> +(defcustom etags-regen-tags-file "TAGS" >> + "Name of the tags file to create inside the project. > > I suggest > > Name of the tags file to create inside the project by `etags-regen-mode'. > >> +(defcustom etags-regen-program-options nil >> + "List of additional options to pass to the etags program." > > I suggest > > List of additional options for etags program invoked by `etags-regen-mode'. > >> +(defcustom etags-regen-regexp-alist nil >> + "Mapping of languages to additional regexps for tags. > > I suggest > > Mapping of languages to etags regexps for `etags-regen-mode'. > >> +(define-minor-mode etags-regen-mode >> + "Generate and update the tags automatically. > > I suggest > > Minor mode to automatically generate and update tags tables. Replaced, thanks. Latest revision attached. Any further comments?