Re: Yet another global/gtags package into elpa..

[Daniel Martín]

Ergus <spacibba@aol.com> writes:

> Hi:
>
> After many years dealing with issues (specially with tramp) with all the
> global/gtags packages around (agtags, gxref, counsel-gtags, ggtags and
> global-tags) I definitively implemented my own simpler one only with
> emacs internal infrastructure, and I am wondering if it may be fine to
> add it to elpa.  The package could be even added to vanilla due to it's
> simplicity and because fully integrates into the emacs ecosystem.
>

Thanks, I think it's a good idea that Emacs has first class support for
GNU Global.  I have some suggestions for your package:

  ;; Package-Requires: ((emacs "28"))

Is there a reason the package requires such a recent version of Emacs?
If it's only because you use string-lines, that logic can be implemented
in terms of split-string, which is available since Emacs 20.1, at least.

  ;;; Commentary:

  ;; GNU Global integration with xref, project and imenu.

It'd be nice to add a ;;; Usage section that describes how to make the
package work.  You can reuse some of the information you have in the
Readme.md file.  For example, you should advertise the gtags-mode-create
command, which generates the tags files for the project.

  (defcustom gtags-mode-lighter "Gtags"
    "Gtags executable."
    :type 'string
    :risky t)

To make things look good in the modeline it's better to add a space
before the lighter, so it should be " Gtags".  Also, the docstring seems
incorrect.

  (add-hook 'after-save-hook #'gtags-mode--after-save-hook nil t)

Could this be optional/configurable? I think some people would prefer to
update the GTAGS database manually via a command and not each time the
file is saved.

  (when (called-interactively-p 'all)
        (message "Couldn't enable gtags-mode. Not root found."))
        ^^^^^^^
        Should be (error ...

This error message is also a bit confusing.  I think it should say
something like "No tags file found" or similar.

Thanks.