Yet another global/gtags package into elpa..

Yet another global/gtags package into elpa..

[Ergus]

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.

The package adds backends for xref, project and imenu with less than 350
lines and fully supporting tramp in an optimized way (checking the
executable availability in remote nodes and respecting the user configs
including connection-local-variables if set). There is just some missing
documentation but I will add it when I have some time.

If you consider that it is fine to add it; please just give me the
steps.

https://github.com/Ergus/global-xref

Best,
Ergus

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

[Michael Albinus]

Ergus <spacibba@aol.com> writes:

> Hi:

Hi,

> If you consider that it is fine to add it; please just give me the
> steps.
>
> https://github.com/Ergus/global-xref

I gave it a cursory reading, some comments (all of them are rather minor).

The Readme.md speaks about global-tags-mode, I guess you mean
global-xref-mode.

--8<---------------cut here---------------start------------->8---
;; Copyright (C) 2022 Jimmy Aguilar Mena

;; Copyright (C) 2022  Jimmy Aguilar Mena
--8<---------------cut here---------------end--------------->8---

This line is doubled, one is sufficient. But if the package is added to
GNU ELPA it will be replaced anyway I guess.

--8<---------------cut here---------------start------------->8---
(defvar global-xref--roots-list nil
  "Full list of project Global root.
The address is absolute on remote hsts.")
--8<---------------cut here---------------end--------------->8---
                                  ^^^^ Typo

--8<---------------cut here---------------start------------->8---
(defconst global-xref--output-format-regex
  "^\\([^ \t]+\\)[ \t]+\\([0-9]+\\)[ \t]+\\([^ \t\]+\\)[ \t]+\\(.*\\)"
--8<---------------cut here---------------end--------------->8---
                                                 ^ Typo
No backslash in front of ].

Instead of " \t" I would prefer "[:blank:]" for better readability, but
this is personal style. Same for "0-9" vs [:digit:]".

--8<---------------cut here---------------start------------->8---
       (let ((criteria `(:machine ,host))
--8<---------------cut here---------------end--------------->8---

Why only the host? The user name could also be relevant. I wouldn't
think too much about, and use just (connection-local-criteria-for-default-directory).

In Emacs 29, you could even use an own :application symbol in order to
distinguish from global Tramp settings.

--8<---------------cut here---------------start------------->8---
    (hack-connection-local-variables-apply
     (connection-local-criteria-for-default-directory))))
--8<---------------cut here---------------end--------------->8---

Here you use it already.

--8<---------------cut here---------------start------------->8---
(defun global-xref--exec-async (command args &optional sentinel)
  "Run COMMAND with ARGS asynchronously and set SENTINEL to process.
Starts an asynchronous process and sets
`global-xref--exec-async-sentinel' as the process sentinel if
SENTINEL is 'nil' or not specified.  Returns the process
handler."
--8<---------------cut here---------------end--------------->8---

We don't quote nil in docstrings. And the function returns the
"process object".

--8<---------------cut here---------------start------------->8---
(defun global-xref--exec-sync (command args &optional sentinel)
  "Run COMMAND with ARGS synchronously, on success call SENTINEL.
Starts a sync process; on success call SENTINEL or
`global-xref--sync-sentinel' if SENTINEL is not specified or
'nil'.  Returns the output of SENTINEL or nil if any error
occurred."
--8<---------------cut here---------------end--------------->8---

The same, we don't quote nil in docstrings.

--8<---------------cut here---------------start------------->8---
(defun global-xref--filter-find-symbol (args symbol creator)
  "Run `global-xref--exec-sync' with ARGS on SYMBOL and filter output with CREATOR.
Returns the results as a list of CREATORS outputs similar to
`mapcar'.  Creator should be a function with 4 input arguments:
name, code, file, line."
--8<---------------cut here---------------end--------------->8---

Creator shall be CREATOR.

> Best,
> Ergus

Best regards, Michael.

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

[Ergus]

Hi:

Very thanks.
>
>
>Why only the host? The user name could also be relevant. I wouldn't
>think too much about, and use just (connection-local-criteria-for-default-directory).
>
Agree, but the problem is that I need to create a unique name for
connection-local-profile-alist entries, and avoid the excessive calls to
executable-find when possible. I am not sure if there is a more standard
approach?

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

[Michael Albinus]

Ergus <spacibba@aol.com> writes:

> Hi:

Hi,

>>Why only the host? The user name could also be relevant. I wouldn't
>>think too much about, and use just (connection-local-criteria-for-default-directory).
>>
> Agree, but the problem is that I need to create a unique name for
> connection-local-profile-alist entries, and avoid the excessive calls to
> executable-find when possible. I am not sure if there is a more standard
> approach?

I would create the profile symbol like

(intern (concat "global-xref--" (file-remote-p default-directory) "-vars"))

There might be some optimizations in case of multi-hops and ports, but
it is a starter.

Best regards, Michael.

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

[Dmitry Gutov]

On 29.03.2022 11:21, Michael Albinus wrote:
> The Readme.md speaks about global-tags-mode, I guess you mean
> global-xref-mode

The naming is a bit unfortunate: it implies a global minor mode for Xref 
functionality, rather than a local minor mode (which it is) for Xref -> 
GNU Global interoperability.

gtags-xref-mode might be a better choice, if it's not taken yet.

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

[Ergus]

On Tue, Mar 29, 2022 at 03:16:30PM +0300, Dmitry Gutov wrote:
>On 29.03.2022 11:21, Michael Albinus wrote:
>>The Readme.md speaks about global-tags-mode, I guess you mean
>>global-xref-mode
>
>The naming is a bit unfortunate: it implies a global minor mode for 
>Xref functionality, rather than a local minor mode (which it is) for 
>Xref -> GNU Global interoperability.
>
Agree and fixed... In general the name `global` for a program like this
is unfortunate... specially when they already have the gtags name which
is clearer and they could do everything with a single executable...

>gtags-xref-mode might be a better choice, if it's not taken yet.
>

https://github.com/Ergus/gtags-xref-mode

My only complain with this is that it is not only xref, but also imenu,
and project...

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

[Dmitry Gutov]

On 29.03.2022 19:07, Ergus wrote:
>> gtags-xref-mode might be a better choice, if it's not taken yet.
>>
> 
> https://github.com/Ergus/gtags-xref-mode

Thanks!

> My only complain with this is that it is not only xref, but also imenu,
> and project...

It's not the end of the world (a bunch of packages have this problem), 
but if you wanted, you could drop 'xref' from the name too. Creating a 
yet-another general purpose GNU Global <-> Emacs integration package.

Some naming options:

gtagstic
ggtags2
gtags-mode (apparently there are no minor modes called exactly that yet)
gtags-ide (inspired by the summary of the 'citre' package)

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

[Ergus]

On Tue, Mar 29, 2022 at 07:16:08PM +0300, Dmitry Gutov wrote:
>On 29.03.2022 19:07, Ergus wrote:
>>>gtags-xref-mode might be a better choice, if it's not taken yet.
>>>
>>
>>https://github.com/Ergus/gtags-xref-mode
>
>Thanks!
>
>>My only complain with this is that it is not only xref, but also imenu,
>>and project...
>
>It's not the end of the world (a bunch of packages have this problem), 
>but if you wanted, you could drop 'xref' from the name too. Creating a 
>yet-another general purpose GNU Global <-> Emacs integration package.
>
>Some naming options:
>
>gtagstic
>ggtags2
>gtags-mode (apparently there are no minor modes called exactly that yet)

This one!!!

https://github.com/Ergus/gtags-mode

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.