GNU bug report logs - #67687
Feature request: automatic tags management

Previous Next

Full log

Message #56 received at 67687 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 67687 <at> debbugs.gnu.org, eskinjp <at> gmail.com, stefankangas <at> gmail.com
Subject: Re: bug#67687: Feature request: automatic tags management
Date: Sat, 30 Dec 2023 09:33:58 +0200

> Date: Sat, 30 Dec 2023 05:05:01 +0200
> Cc: 67687 <at> debbugs.gnu.org, eskinjp <at> gmail.com, stefankangas <at> gmail.com
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> 
> >> +(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.

> >> +;;;###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.

> >> +     (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?

> 
> >> +(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?

> >> +(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?

> +(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.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.