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

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]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

Hello,

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.

Good day,

Björn


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Define-bug-reference-url-format-as-custom-type.patch --]
[-- Type: text/x-patch, Size: 1774 bytes --]

From 0a0f4062c41de25225df6b216470141d3537308c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Bidar?= <bjorn.bidar@thaodan.de>
Date: Tue, 20 Aug 2024 16:05:31 +0300
Subject: [PATCH 1/3] Define bug-reference-url-format as custom type

* 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
it as a custom type to check the type and group it under the other
bug-reference-mode settings
---
 lisp/progmodes/bug-reference.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/bug-reference.el b/lisp/progmodes/bug-reference.el
index 3bcfc213fc6..815e51aef73 100644
--- a/lisp/progmodes/bug-reference.el
+++ b/lisp/progmodes/bug-reference.el
@@ -48,7 +48,7 @@ bug-reference-map
   "C-c RET"   #'bug-reference-push-button)
 
 ;; E.g., "https://gcc.gnu.org/PR%s"
-(defvar bug-reference-url-format nil
+(defcustom bug-reference-url-format nil
   "Format used to turn a bug number into a URL.
 The bug number is supplied as a string, so this should have a single %s.
 This can also be a function designator; it is called without arguments
@@ -62,7 +62,11 @@ bug-reference-url-format
 If you set it to a symbol in the file Local Variables section,
 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'.")
+so that it is considered safe, see `enable-local-variables'."
+ :type '(choice (function)
+                (string))
+ :version "30.1"
+ :group 'bug-reference)
 
 ;;;###autoload
 (put 'bug-reference-url-format 'safe-local-variable
-- 
2.45.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Define-bug-reference-forge-alist-as-custom-type.patch --]
[-- Type: text/x-patch, Size: 3631 bytes --]

From 5cad0f9d1cb982f03a60c494f0a363056feab688 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Bidar?= <bjorn.bidar@thaodan.de>
Date: Tue, 20 Aug 2024 17:09:14 +0300
Subject: [PATCH 2/3] Define bug-reference-forge-alist as custom type

* lisp/progmodes/bug-reference.el (bug-reference-forge-alist):
Define as custom type. The manual and the documentation string
refere to it like it is a custom variable.
Further defcustom will also ensure that the variable is the right type
when the user modifies it.
---
 lisp/progmodes/bug-reference.el | 49 +++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/lisp/progmodes/bug-reference.el b/lisp/progmodes/bug-reference.el
index 815e51aef73..9f293754c78 100644
--- a/lisp/progmodes/bug-reference.el
+++ b/lisp/progmodes/bug-reference.el
@@ -104,6 +104,37 @@ bug-reference-bug-regexp
 ;;;###autoload
 (put 'bug-reference-bug-regexp 'safe-local-variable 'stringp)
 
+
+(defcustom bug-reference-forge-alist
+  '(("github.com"       github    "https")
+    ("gitea.com"        gitea     "https")
+    ("codeberg.org"     gitea     "https")
+    ("gitlab.com"       gitlab    "https")
+    ("framagit.org"     gitlab    "https")
+    ("salsa.debian.org" gitlab    "https")
+    ("sr.ht"            sourcehut "https"))
+  "An alist of forge instances.
+Each entry has the form (HOST-DOMAIN FORGE-TYPE PROTOCOL).
+HOST-DOMAIN is the host- and domain name, e.g., gitlab.com,
+salsa.debian.org, or sr.ht.
+FORGE-TYPE is the type of the forge, e.g., gitlab, gitea,
+sourcehut, or github.
+PROTOCOL is the protocol for accessing the forge's issue tracker,
+usually \"https\" but for self-hosted forge instances not
+accessible via the internet it might also be \"http\"."
+  :type '(alist :key-type (string :tag "Host-Domain")
+                :value-type (group (choice :tag "Forge-Type"
+                                           (const :tag "Github" github)
+                                           (const :tag "Gitea" gitea)
+                                           (const :tag "Gitlab" gitlab)
+                                           (const :tag "Sourcehut" sourcehut)
+                                           (symbol))
+                                   (choice :tag "Protocol"
+                                           (const "https") (string))))
+  :group 'bug-reference
+  :version "30.1")
+
+
 (defun bug-reference-set-overlay-properties ()
   "Set properties of bug reference overlays."
   (put 'bug-reference 'evaporate t)
@@ -240,24 +271,6 @@ bug-reference--setup-from-vc-alist
 from a few default entries, and the value of
 `bug-reference-forge-alist'.")
 
-(defvar bug-reference-forge-alist
-  '(("github.com"       github    "https")
-    ("gitea.com"        gitea     "https")
-    ("codeberg.org"     gitea     "https")
-    ("gitlab.com"       gitlab    "https")
-    ("framagit.org"     gitlab    "https")
-    ("salsa.debian.org" gitlab    "https")
-    ("sr.ht"            sourcehut "https"))
-  "An alist of forge instances.
-Each entry has the form (HOST-DOMAIN FORGE-TYPE PROTOCOL).
-HOST-DOMAIN is the host- and domain name, e.g., gitlab.com,
-salsa.debian.org, or sr.ht.
-FORGE-TYPE is the type of the forge, e.g., gitlab, gitea,
-sourcehut, or github.
-PROTOCOL is the protocol for accessing the forge's issue tracker,
-usually \"https\" but for self-hosted forge instances not
-accessible via the internet it might also be \"http\".")
-
 (cl-defgeneric bug-reference--build-forge-setup-entry
     (host-domain forge-type protocol)
   "Build an entry for `bug-reference--setup-from-vc-alist'.
-- 
2.45.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Set-custom-group-of-bug-reference-bug-regexp.patch --]
[-- Type: text/x-patch, Size: 838 bytes --]

From 5498829ad845ad7763d25f417be52233cc707753 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Bidar?= <bjorn.bidar@thaodan.de>
Date: Tue, 20 Aug 2024 17:41:20 +0300
Subject: [PATCH 3/3] ; Set custom group of bug-reference-bug-regexp

---
 lisp/progmodes/bug-reference.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/progmodes/bug-reference.el b/lisp/progmodes/bug-reference.el
index 9f293754c78..00d8f5d506c 100644
--- a/lisp/progmodes/bug-reference.el
+++ b/lisp/progmodes/bug-reference.el
@@ -97,6 +97,7 @@ bug-reference-bug-regexp
 outside the bounds of subexpressions 1 and then don't contribute
 to the highlighted and clickable region."
   :type 'regexp
+  :group 'bug-reference
   ; 24.3: defconst -> defcustom
   ; 28.1: contract about subexpression 1 defines the overlay region.
   :version "28.1")
-- 
2.45.2

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]

I noticed that there are other variable such as
bug-reference-setup-from-mail-alist that should be change similarly.

Would it make sense to group the changes to convert them to defcustom in
one patch or is one patch per variable better?

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]

I noticed that there are other variable such as
bug-reference-setup-from-mail-alist that should be change similarly.

Would it make sense to group the changes to convert them to defcustom in
one patch or is one patch per variable better?

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

[Eli Zaretskii]

> 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.

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.

> -(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.

> + :group 'bug-reference)

This is redundant (here and elsewhere in the patches), since the
package name will supply the group.

> +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.

Finally, I think this warrants a NEWS entry.

Thanks.

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

[Eli Zaretskii]

> Date: Tue, 20 Aug 2024 20:40: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 there are other variable such as
> bug-reference-setup-from-mail-alist that should be change similarly.

Which ones, specifically?

And let's get Tassilo (CC'ed) on board of this discussion.

> Would it make sense to group the changes to convert them to defcustom in
> one patch or is one patch per variable better?

I prefer a single patch, thanks.

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]

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.

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]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Tue, 20 Aug 2024 20:40: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 there are other variable such as
>> bug-reference-setup-from-mail-alist that should be change similarly.
>
> Which ones, specifically?

Besides the one mentioned above bug-reference-setup-from-irc-alist and bug-reference-auto-setup-functions. 
The latter to e.g. the user providing their own functions.

> And let's get Tassilo (CC'ed) on board of this discussion.
>
>> Would it make sense to group the changes to convert them to defcustom in
>> one patch or is one patch per variable better?
>
> I prefer a single patch, thanks.

Ok great.

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

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

Hi all,

>> I noticed that there are other variable such as
>> bug-reference-setup-from-mail-alist that should be change similarly.
>
> Which ones, specifically?
>
> And let's get Tassilo (CC'ed) on board of this discussion.

As Eli already mentioned, bug-reference-url-format is usually set via a
file local variables section or programatically so neither a defcustom
nor a default value makes sense.

Wrt. bug-reference-setup-from-mail-alist,
bug-reference-setup-from-irc-alist, and
bug-reference-setup-from-vc-alist: yes, they could be defcustoms but
their entries can all (and are likely to) contain functions which are
hard to specify in the defcustom interface.  I've thought it wouldn't be
needed.  After all, bug-reference is a programmer's tool.

Wrt. bug-reference-forge-alist: if it became a defcustom and a user
would set it, she wouldn't see updates (like support for some new forge)
in its default value anymore because their old saved custom value
overrides the new default value.  It's much better to add new entries
programatically using add-to-list or push/cl-pushnew.  Of course, one
could split the variable in some *-default-alist defvar and a defcustom
*-alist where the latter is meant for user customization but I think
it's not worth the added complexity here.

So I'd rather keep it as-is.

Bye,
  Tassilo

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

[Eli Zaretskii]

> From: Björn Bidar <bjorn.bidar@thaodan.de>
> Cc: 72735@debbugs.gnu.org
> Date: Tue, 20 Aug 2024 22:39:18 +0300
> 
> >> +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.

It might be enough to say that those symbols are the only predefined
ones.  The text as it is leaves the impression that they are only
examples.  Also, something should be said regarding the differences
between the symbols: why does the user need to specify these symbols
and what could happen if they use a wrong symbol?

> > 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?

Just the fact that they are now user options, that's all.

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

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: Björn Bidar <bjorn.bidar@thaodan.de>,
>   72735@debbugs.gnu.org
> Date: Wed, 21 Aug 2024 07:20:27 +0200
> 
> As Eli already mentioned, bug-reference-url-format is usually set via a
> file local variables section or programatically so neither a defcustom
> nor a default value makes sense.
> 
> Wrt. bug-reference-setup-from-mail-alist,
> bug-reference-setup-from-irc-alist, and
> bug-reference-setup-from-vc-alist: yes, they could be defcustoms but
> their entries can all (and are likely to) contain functions which are
> hard to specify in the defcustom interface.  I've thought it wouldn't be
> needed.  After all, bug-reference is a programmer's tool.
> 
> Wrt. bug-reference-forge-alist: if it became a defcustom and a user
> would set it, she wouldn't see updates (like support for some new forge)
> in its default value anymore because their old saved custom value
> overrides the new default value.  It's much better to add new entries
> programatically using add-to-list or push/cl-pushnew.  Of course, one
> could split the variable in some *-default-alist defvar and a defcustom
> *-alist where the latter is meant for user customization but I think
> it's not worth the added complexity here.
> 
> So I'd rather keep it as-is.

Thanks.

So, Björn, now you get to try to convince Tassilo that making some of
these variables defcustoms does make sense.  IOW, what are the
problems in the current situation that prompted you to suggest these
changes?