bug#72735: 31.0.50; [PATCH] Make more bug-reference variables customizeable

["Björn Bidar via Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Tue, 20 Aug 2024 18:34:38 +0300
>> From:  Björn Bidar via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> I noticed that some of the variables that can be modified
>> by the user or as mentioned as such were not defined as custom variable.
>> These patches to so and correct the group of one of the existing
>> defcustom.
>> 
>> The patches don't change any of the existing functionality
>> but only document these variables better and make them easier to modify.
>> Because of that I set version to Emacs 30.1. Please tell if that's ok.
>
> The emacs-30 release branch is closed to enhancements, it only
> receives bugfix changes and improvements in documentation.  Converting
> a defvar to a defcustom is not just a documentation change, it
> radically changes how the variable is initialized and set.  So this is
> not appropriate for the release branch.

OK I wasn't aware that there's such a difference for existing user that
don't use defcustom.

> Please see several comments below.
>
>> * lisp/progmodes/bug-reference.el (bug-reference-url-format): Define as
>> custom type. The manual and the documentation string talk like the
>              ^^
>> variable like is a custom variable. It does make sense to define
>                                     ^^
> Our convention is to leave two spaces between sentences in comments,
> strings, and commit log messages.  There are several places in the
> patch where you left only one space.

Will fix, in prior changes to the file there were changes with only one space.

>> -(defvar bug-reference-url-format nil
>> +(defcustom bug-reference-url-format nil
>>    "Format used to turn a bug number into a URL.
>
> I wonder how this makes sense as a defcustom, since this variable must
> be file-local, AFAIU.  In any case, the doc string should explain the
> semantics of the nil value.

The doc string says it must be local however I see no reason to not be
able to have a global function that returns a default if no setting is
found for the file. E.g. it's common to have something like
keyword#bugnumber where each keyword would refer to a different
bugtracker. 

>> + :group 'bug-reference)
>
> This is redundant (here and elsewhere in the patches), since the
> package name will supply the group.

ok,.

>> +FORGE-TYPE is the type of the forge, e.g., gitlab, gitea,
>> +sourcehut, or github.
>
> This leaves it unsaid how these symbols are used, and what symbols are
> recognized.

Does the original text not say that already? The choices over all the
existing symbols which are accepted, if it doesn't make sense to
e.g. add a custom symbol to extend the mode than removing the (symbol)
choice is good.

> Finally, I think this warrants a NEWS entry.

Should the news entry explain similarly that the variables can be set
with defcustom and have improved documentation?

> Thanks.