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.