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

Previous Next

Full log

View this message in rfc822 format

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 67687 <at> debbugs.gnu.org, eskinjp <at> gmail.com, stefankangas <at> gmail.com
Subject: bug#67687: Feature request: automatic tags management
Date: Sun, 31 Dec 2023 01:43:25 +0200

[Message part 1 (text/plain, inline)]

On 30/12/2023 09:33, Eli Zaretskii wrote:
>> 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.

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?

[etags-regen-v5.diff (text/x-patch, attachment)]

Previous Next

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