[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Basil L. Contovounesios
Subject: Re: RFC: Automatic setup for bug-reference-mode
Date: Sun, 14 Jun 2020 15:56:27 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)

Tassilo Horn <tsdh@gnu.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>>> +           (url (pcase backend
>>> +                  ('Git (string-trim
>> This needs (eval-when-compile (require 'subr-x)).
> Or simply remove the "p" from "pcase", I guess.

I was referring to your use of string-trim.

>>> +                         (shell-command-to-string
>>> +                          "git ls-remote --get-url"))))))
>> Doesn't VC provide a robust way to get output from Git?
> vc-git--call maybe?

Perhaps; I'm not that familiar with VC.

>>> +      (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?
> Yes.  Is is generally better to use shy groups if we aren't going to do
> anything with the groups anyway?

Yes, for the negligible potential performance gain, so that there's one
less group number taken, and more importantly so the reader isn't left
wondering why we're capturing this.

Personally, I often just go with what rx does, either by writing rx
forms directly or by manually copying its expansion into the program, as
that way I'm less likely to make a mistake.

>>>  ;;;###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.
> Indeed.  Obviously it should have done what the docstring says.
>> Instead of changing the function's arglist, would it be any worse to
>> do the following?
>>   (ignore-errors (vc-responsible-backend ...))
> IMHO, that it errors if no backend is found is the actual error but we
> cannot change that anymore.  And to me "what backend would this file
> use" is a very common question.  Even vc.el itself encodes that (dolist
> (backend vc-handled-backends) ...) form again in
> `vc-backend-for-registration' in order not to trigger the error
> semantics of `vc-responsible-backend'.
> But that's not important to me.  I can also leave it as it is.

FWIW I'm happy with the optional argument you suggest.

> PS: I like your style of suggesting improvements using questions which
> all have a positive answer.  Did you visit a lecture of Stefan? ;-)

No, but I wish. ;)



reply via email to

[Prev in Thread] Current Thread [Next in Thread]