From: "Basil L. Contovounesios" <contovob@tcd.ie>
To: emacs-devel@gnu.org
Cc: Stefan Monnier <monnier@iro.umontreal.ca>
Subject: Re: RFC: Automatic setup for bug-reference-mode
Date: Sun, 14 Jun 2020 13:13:57 +0100 [thread overview]
Message-ID: <87blllx2t6.fsf@tcd.ie> (raw)
In-Reply-To: <87r1uihtsu.fsf@gnu.org> (Tassilo Horn's message of "Sun, 14 Jun 2020 11:37:37 +0200")
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
next prev parent reply other threads:[~2020-06-14 12:13 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-14 9:37 RFC: Automatic setup for bug-reference-mode Tassilo Horn
2020-06-14 12:13 ` Basil L. Contovounesios [this message]
2020-06-14 12:56 ` Tassilo Horn
2020-06-14 14:56 ` Basil L. Contovounesios
2020-06-14 14:22 ` Stefan Monnier
2020-06-14 15:18 ` Tassilo Horn
2020-06-14 16:30 ` Tassilo Horn
2020-06-14 18:08 ` Basil L. Contovounesios
2020-06-14 18:40 ` Stefan Monnier
2020-06-14 18:57 ` Basil L. Contovounesios
2020-06-14 19:43 ` Tassilo Horn
2020-06-14 19:41 ` Dmitry Gutov
2020-06-14 20:39 ` Tassilo Horn
2020-06-14 20:51 ` Dmitry Gutov
2020-06-14 21:03 ` Basil L. Contovounesios
2020-06-15 6:23 ` VC repository-url command (was: RFC: Automatic setup for bug-reference-mode) Tassilo Horn
2020-06-15 11:33 ` VC repository-url command Basil L. Contovounesios
2020-06-15 9:56 ` RFC: Automatic setup for bug-reference-mode Stephen Leake
2020-06-15 10:21 ` Tassilo Horn
2020-06-17 21:35 ` Juri Linkov
2020-06-17 22:10 ` Dmitry Gutov
2020-06-18 6:06 ` Tassilo Horn
2020-06-18 9:46 ` Dmitry Gutov
2020-06-18 13:37 ` Tassilo Horn
2020-06-18 14:28 ` Dmitry Gutov
2020-06-15 10:57 ` Tassilo Horn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87blllx2t6.fsf@tcd.ie \
--to=contovob@tcd.ie \
--cc=emacs-devel@gnu.org \
--cc=monnier@iro.umontreal.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.