Re: RFC: Automatic setup for bug-reference-mode

[Basil L. Contovounesios]

Tassilo Horn <tsdh@gnu.org> writes:

> Attach is my first attempt at doing so and I'd welcome comments.

Thanks, just some minor comments from me.

[...]

> modified   lisp/progmodes/bug-reference.el
> @@ -60,6 +60,7 @@ bug-reference-url-format
>  you need to add a `bug-reference-url-format' property to it:
>  \(put \\='my-bug-reference-url-format \\='bug-reference-url-format t)
>  so that it is considered safe, see `enable-local-variables'.")
> +(make-variable-buffer-local 'bug-reference-url-format)

Nit: You can use defvar-local instead.

>  ;;;###autoload
>  (put 'bug-reference-url-format 'safe-local-variable
> @@ -75,6 +76,7 @@ bug-reference-bug-regexp
>    :type 'regexp
>    :version "24.3"                    ; previously defconst
>    :group 'bug-reference)
> +(make-variable-buffer-local 'bug-reference-bug-regexp)

Nit: You can use ':local t' instead.

>  ;;;###autoload
>  (put 'bug-reference-bug-regexp 'safe-local-variable 'stringp)
> @@ -139,6 +141,139 @@ bug-reference-push-button
>       (when url
>         (browse-url url))))))
>  
> +(defcustom bug-reference-setup-functions nil
> +  "A list of function for setting up bug-reference mode.

Nit: Either "Bug-Reference Mode" or "`bug-reference-mode'".

> +A setup function should return non-nil if it set
> +`bug-reference-bug-regexp' and `bug-reference-url-format'
> +appropiately for the current buffer.  The functions are called in

"appropriately"

> +sequence stopping as soon as one signalled a successful setup.

The Emacs sources predominantly use the spelling "signaled".

> +They are only called if the two variables aren't set already,
> +e.g., by a local variables section.
> +
> +Also see `bug-reference-default-setup-functions'.
> +
> +The `bug-reference-setup-functions' take preference over

"take precedence"

> +`bug-reference-default-setup-functions', i.e., they are
> +called before the latter."
> +  :type '(list function)

Either 'hook or '(repeat function).

> +  :version "28.1"
> +  :group 'bug-reference)
> +
> +(defun bug-reference-try-setup-from-vc ()
> +  "Try setting up `bug-reference-bug-regexp' and
> +`bug-reference-url-format' from the version control system of the
> +current file."

Nit: Please follow the recommendation in "(elisp) Documentation Tips" of
fitting the first sentence on a single line.

> +  (when (buffer-file-name)
> +    (let* ((backend (vc-responsible-backend (buffer-file-name) t))

Nit: Any reason not to use the buffer-local variable buffer-file-name
instead?

> +           (url (pcase backend
> +                  ('Git (string-trim

This needs (eval-when-compile (require 'subr-x)).

> +                         (shell-command-to-string
> +                          "git ls-remote --get-url"))))))

Doesn't VC provide a robust way to get output from Git?

> +      (cl-flet ((maybe-set (url-rx bug-rx bug-url-fmt)
> +                           (when (string-match url-rx url)
> +                             (setq bug-reference-bug-regexp bug-rx)
> +                             (setq bug-reference-url-format
> +                                   (if (functionp bug-url-fmt)
> +                                       (funcall bug-url-fmt)
> +                                     bug-url-fmt)))))
> +        (when (and url
> +                   ;; If there's a space in the url, it's propably an
> +                   ;; error message.
> +                   (not (string-match-p "[[:space:]]" url)))
> +          (or
> +           ;; GNU projects on savannah.  FIXME: Only a fraction of
> +           ;; them uses debbugs.
> +           (maybe-set "git\\.\\(sv\\|savannah\\)\\.gnu\\.org:"
                                ^^^
Nit: Can this be a shy group?

> +                      "\\([Bb]ug ?#?\\)\\([0-9]+\\(?:#[0-9]+\\)?\\)"
> +                      "https://debbugs.gnu.org/%s";)
> +           ;; GitHub projects.  Here #17 may refer to either an issue
> +           ;; or a pull request but visiting the issue/17 web page
> +           ;; will automatically redirect to the pull/17 page if 17 is
> +           ;; a PR.  Explicit user/project#17 links to possibly
> +           ;; different projects are also supported.
> +           (maybe-set
> +            "[/@]github.com[/:]\\([.A-Za-z0-9_/-]+\\)\\.git"
> +            "\\([.A-Za-z0-9_/-]+\\)?\\(?:#\\)\\([0-9]+\\)"
                                       ^^^^^^^^^
Nit: Why is this in a group?

> +            (lambda ()
> +              (let ((ns-project (match-string 1 url)))
> +                (lambda ()
> +                  (concat "https://github.com/";
> +                          (or
> +                           ;; Explicit user/proj#18 link.
> +                           (match-string 1)
> +                           ns-project)
> +                          "/issues/"
> +                          (match-string 2))))))
> +           ;; GitLab projects.  Here #18 is an issue and !17 is a
> +           ;; merge request.  Explicit namespace/project#18 references
> +           ;; to possibly different projects are also supported.
> +           (maybe-set
> +            "[/@]gitlab.com[/:]\\([.A-Za-z0-9_/-]+\\)\\.git"
> +            "\\(?1:[.A-Za-z0-9_/-]+\\)?\\(?3:#\\|!\\)\\(?2:[0-9]+\\)"

Nit: Isn't this the same as "\\(?3:[!#]\\)"?

> +            (lambda ()
> +              (let ((ns-project (match-string 1 url)))
> +                (lambda ()
> +                  (concat "https://gitlab.com/";
> +                          (or (match-string 1)
> +                              ns-project)
> +                          "/-/"
> +                          (if (string= (match-string 3) "#")
> +                              "issues/"
> +                            "merge_requests/")
> +                          (match-string 2))))))))))))
> +
> +(defun bug-reference-try-setup-from-gnus ()
> +  (when (and (memq major-mode '(gnus-summary-mode gnus-article-mode))
> +             (boundp 'gnus-newsgroup-name)
> +             gnus-newsgroup-name)

  (and (derived-mode-p 'gnus-summary-mode 'gnus-article-mode)
       (bound-and-true-p gnus-newsgroup-name))

or

  (and (derived-mode-p 'gnus-mode)
       (bound-and-true-p gnus-newsgroup-name))

[...]

> +(defun bug-reference--init ()
> +  "Initialize `bug-reference-mode'."
> +  (progn

Nit: Isn't this progn redundant?

> +    ;; Automatic setup only if the variables aren't already set, e.g.,
> +    ;; by a local variables section in the file.
> +    (unless (and bug-reference-bug-regexp
> +                 bug-reference-url-format)
> +      (or
> +       (with-demoted-errors
> +           "Error while running bug-reference-setup-functions: %S"
> +         (run-hook-with-args-until-success
> +          'bug-reference-setup-functions))
> +       (with-demoted-errors
> +           "Error while running bug-reference-default-setup-functions: %S"
> +         (run-hook-with-args-until-success
> +          'bug-reference-default-setup-functions))))
> +    (jit-lock-register #'bug-reference-fontify)))

[...]

>  ;;;###autoload
> -(defun vc-responsible-backend (file)
> +(defun vc-responsible-backend (file &optional no-error)
>    "Return the name of a backend system that is responsible for FILE.
>  
>  If FILE is already registered, return the
> @@ -967,7 +967,10 @@ vc-responsible-backend
>  
>  Note that if FILE is a symbolic link, it will not be resolved --
>  the responsible backend system for the symbolic link itself will
> -be reported."
> +be reported.
> +
> +If NO-ERROR is nil, signal an error that no VC backend is
> +responsible for the given file."
>    (or (and (not (file-directory-p file)) (vc-backend file))
>        (catch 'found
>       ;; First try: find a responsible backend.  If this is for registration,

NO-ERROR seems to be a no-op in this patch.  Instead of changing the
function's arglist, would it be any worse to do the following?

  (ignore-errors (vc-responsible-backend ...))

Thanks,

-- 
Basil