bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[john muhl]

Tags: patch

For some silly reason the lua-ts-{inferior,luacheck}-program
options ended up with an option for “None”. Obviously setting your
Lua interpreter or linter program to nil is not going to work.

Any chance it could make it into emacs-30?

Thanks.

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[john muhl]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Remove-nil-from-some-lua-ts-mode-options.patch --]
[-- Type: text/x-patch, Size: 1163 bytes --]

From 9875334458c9b6820eeb1f5762c994e52cc9cdba Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
Date: Wed, 6 Nov 2024 14:56:24 -0600
Subject: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

* lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
(lua-ts-inferior-program): Remove 'nil' as it is never a valid
choice.  (Bug#74235)
---
 lisp/progmodes/lua-ts-mode.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
index 20bc1f3e158..1ab0b29608f 100644
--- a/lisp/progmodes/lua-ts-mode.el
+++ b/lisp/progmodes/lua-ts-mode.el
@@ -72,7 +72,7 @@ lua-ts-indent-offset
 
 (defcustom lua-ts-luacheck-program "luacheck"
   "Location of the Luacheck program."
-  :type '(choice (const :tag "None" nil) string)
+  :type 'string
   :version "30.1")
 
 (defcustom lua-ts-inferior-buffer "*Lua*"
@@ -83,7 +83,7 @@ lua-ts-inferior-buffer
 
 (defcustom lua-ts-inferior-program "lua"
   "Program to run in the inferior Lua process."
-  :type '(choice (const :tag "None" nil) string)
+  :type 'string
   :version "30.1")
 
 (defcustom lua-ts-inferior-options '("-i")
-- 
2.47.0


[-- Attachment #2: Type: text/plain, Size: 324 bytes --]



john muhl <jm@pub.pink> writes:

> Tags: patch
>
> For some silly reason the lua-ts-{inferior,luacheck}-program
> options ended up with an option for “None”. Obviously setting your
> Lua interpreter or linter program to nil is not going to work.
>
> Any chance it could make it into emacs-30?
>
> Thanks.

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[Eli Zaretskii]

> From: john muhl <jm@pub.pink>
> Date: Wed, 06 Nov 2024 15:22:34 -0600
> 
> * lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
> (lua-ts-inferior-program): Remove 'nil' as it is never a valid
> choice.  (Bug#74235)

IMO, the use of these options in the code is not clean: if these
programs are not installed, the commands which reference these options
will signal errors whose diagnostic might not clearly indicate the
root cause.  I would like to see a cleaner handling of such
contingencies by this mode.  Does this make sense, and if so, would
you like to suggest changes to that effect?

>  (defcustom lua-ts-luacheck-program "luacheck"
>    "Location of the Luacheck program."
> -  :type '(choice (const :tag "None" nil) string)
> +  :type 'string
>    :version "30.1")

I think if we require this to be the name of a program, 'file' is a
better value for :type, because it allows completion if the user types
an absolute file name.

>  (defcustom lua-ts-inferior-buffer "*Lua*"
> @@ -83,7 +83,7 @@ lua-ts-inferior-buffer
>  
>  (defcustom lua-ts-inferior-program "lua"
>    "Program to run in the inferior Lua process."
> -  :type '(choice (const :tag "None" nil) string)
> +  :type 'string
>    :version "30.1")
>  
>  (defcustom lua-ts-inferior-options '("-i")
> -- 
> 2.47.0
> 
> john muhl <jm@pub.pink> writes:
> 
> > Tags: patch
> >
> > For some silly reason the lua-ts-{inferior,luacheck}-program
> > options ended up with an option for “None”. Obviously setting your
> > Lua interpreter or linter program to nil is not going to work.
> >
> > Any chance it could make it into emacs-30?

lua-ts-mode is new in Emacs 30, so we can install these changes on the
release branch, as a stopgap.  But I'd like to see a cleaner solution
installed later, even if we decide to install that on master due to
its complexity.

Thanks.

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[john muhl]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: john muhl <jm@pub.pink>
>> Date: Wed, 06 Nov 2024 15:22:34 -0600
>> 
>> * lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
>> (lua-ts-inferior-program): Remove 'nil' as it is never a valid
>> choice.  (Bug#74235)
>
> IMO, the use of these options in the code is not clean: if these
> programs are not installed, the commands which reference these options
> will signal errors whose diagnostic might not clearly indicate the
> root cause.  I would like to see a cleaner handling of such
> contingencies by this mode.  Does this make sense, and if so, would
> you like to suggest changes to that effect?

Good point. I’ll work on that very soon.

>>  (defcustom lua-ts-luacheck-program "luacheck"
>>    "Location of the Luacheck program."
>> -  :type '(choice (const :tag "None" nil) string)
>> +  :type 'string
>>    :version "30.1")
>
> I think if we require this to be the name of a program, 'file' is a
> better value for :type, because it allows completion if the user types
> an absolute file name.

Makes sense.

>>  (defcustom lua-ts-inferior-buffer "*Lua*"
>> @@ -83,7 +83,7 @@ lua-ts-inferior-buffer
>>  
>>  (defcustom lua-ts-inferior-program "lua"
>>    "Program to run in the inferior Lua process."
>> -  :type '(choice (const :tag "None" nil) string)
>> +  :type 'string
>>    :version "30.1")
>>  
>>  (defcustom lua-ts-inferior-options '("-i")
>> -- 
>> 2.47.0
>> 
>> john muhl <jm@pub.pink> writes:
>> 
>> > Tags: patch
>> >
>> > For some silly reason the lua-ts-{inferior,luacheck}-program
>> > options ended up with an option for “None”. Obviously setting your
>> > Lua interpreter or linter program to nil is not going to work.
>> >
>> > Any chance it could make it into emacs-30?
>
> lua-ts-mode is new in Emacs 30, so we can install these changes on the
> release branch, as a stopgap.  But I'd like to see a cleaner solution
> installed later, even if we decide to install that on master due to
> its complexity.
>
> Thanks.

Then let’s put this one on hold and I’ll work on the improvements
you suggest. Hopefully it doesn’t require significant changes and
we can skip the stopgap and get it all into 30.

Thanks for the help.

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[john muhl]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: john muhl <jm@pub.pink>
>> Date: Wed, 06 Nov 2024 15:22:34 -0600
>> 
>> * lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
>> (lua-ts-inferior-program): Remove 'nil' as it is never a valid
>> choice.  (Bug#74235)
>
> IMO, the use of these options in the code is not clean: if these
> programs are not installed, the commands which reference these options
> will signal errors whose diagnostic might not clearly indicate the
> root cause.  I would like to see a cleaner handling of such
> contingencies by this mode.  Does this make sense, and if so, would
> you like to suggest changes to that effect?

The only idea I’ve come up with so far is showing a warning that
mentions the incorrectly set option. Does that sound like what you
have in mind? If not I’d appreciate some more explicit guidance.

Thanks.

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[Eli Zaretskii]

> From: john muhl <jm@pub.pink>
> Cc: 74235@debbugs.gnu.org
> Date: Fri, 08 Nov 2024 08:57:37 -0600
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: john muhl <jm@pub.pink>
> >> Date: Wed, 06 Nov 2024 15:22:34 -0600
> >> 
> >> * lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
> >> (lua-ts-inferior-program): Remove 'nil' as it is never a valid
> >> choice.  (Bug#74235)
> >
> > IMO, the use of these options in the code is not clean: if these
> > programs are not installed, the commands which reference these options
> > will signal errors whose diagnostic might not clearly indicate the
> > root cause.  I would like to see a cleaner handling of such
> > contingencies by this mode.  Does this make sense, and if so, would
> > you like to suggest changes to that effect?
> 
> The only idea I’ve come up with so far is showing a warning that
> mentions the incorrectly set option. Does that sound like what you
> have in mind? If not I’d appreciate some more explicit guidance.

How about signaling user-error from the commands that invoke those
programs, when they are missing?

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[john muhl]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: john muhl <jm@pub.pink>
>> Cc: 74235@debbugs.gnu.org
>> Date: Fri, 08 Nov 2024 08:57:37 -0600
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: john muhl <jm@pub.pink>
>> >> Date: Wed, 06 Nov 2024 15:22:34 -0600
>> >> 
>> >> * lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
>> >> (lua-ts-inferior-program): Remove 'nil' as it is never a valid
>> >> choice.  (Bug#74235)
>> >
>> > IMO, the use of these options in the code is not clean: if these
>> > programs are not installed, the commands which reference these options
>> > will signal errors whose diagnostic might not clearly indicate the
>> > root cause.  I would like to see a cleaner handling of such
>> > contingencies by this mode.  Does this make sense, and if so, would
>> > you like to suggest changes to that effect?
>> 
>> The only idea I’ve come up with so far is showing a warning that
>> mentions the incorrectly set option. Does that sound like what you
>> have in mind? If not I’d appreciate some more explicit guidance.
>
> How about signaling user-error from the commands that invoke those
> programs, when they are missing?

Agree that’s better than a warning. It seems flymake already
handles the case where it can’t find the requested program so I
left that part as is.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-some-lua-ts-mode-options-Bug-74235.patch --]
[-- Type: text/x-patch, Size: 6048 bytes --]

From 3e49aa1086f2498ccbad396cf8c799c57919847e Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
Date: Sat, 9 Nov 2024 11:01:45 -0600
Subject: [PATCH] ; Fix some 'lua-ts-mode' options (Bug#74235)

* lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
(lua-ts-inferior-program): Switch to 'file' type and remove 'nil'
as a choice.
(lua-ts-inferior-lua): Ensure 'lua-ts-inferior-program' is a
valid executable.
---
 lisp/progmodes/lua-ts-mode.el | 85 ++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 41 deletions(-)

diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
index 4ea453c9b65..2323461ed4c 100644
--- a/lisp/progmodes/lua-ts-mode.el
+++ b/lisp/progmodes/lua-ts-mode.el
@@ -72,7 +72,7 @@ lua-ts-indent-offset
 
 (defcustom lua-ts-luacheck-program "luacheck"
   "Location of the Luacheck program."
-  :type '(choice (const :tag "None" nil) string)
+  :type 'file
   :version "30.1")
 
 (defcustom lua-ts-inferior-buffer "*Lua*"
@@ -83,7 +83,7 @@ lua-ts-inferior-buffer
 
 (defcustom lua-ts-inferior-program "lua"
   "Program to run in the inferior Lua process."
-  :type '(choice (const :tag "None" nil) string)
+  :type 'file
   :version "30.1")
 
 (defcustom lua-ts-inferior-options '("-i")
@@ -632,47 +632,50 @@ lua-ts-flymake-luacheck
 (defun lua-ts-inferior-lua ()
   "Run a Lua interpreter in an inferior process."
   (interactive)
-  (unless (comint-check-proc lua-ts-inferior-buffer)
-    (apply #'make-comint-in-buffer
-           (string-replace "*" "" lua-ts-inferior-buffer)
-           lua-ts-inferior-buffer
-           lua-ts-inferior-program
-           lua-ts-inferior-startfile
-           lua-ts-inferior-options)
-    (when lua-ts-inferior-history
+  (if (or (not lua-ts-inferior-program)
+          (not (executable-find lua-ts-inferior-program)))
+      (user-error "You must set `lua-ts-inferior-program' to use this command")
+    (unless (comint-check-proc lua-ts-inferior-buffer)
+      (apply #'make-comint-in-buffer
+             (string-replace "*" "" lua-ts-inferior-buffer)
+             lua-ts-inferior-buffer
+             lua-ts-inferior-program
+             lua-ts-inferior-startfile
+             lua-ts-inferior-options)
+      (when lua-ts-inferior-history
         (set-process-sentinel (get-buffer-process lua-ts-inferior-buffer)
                               'lua-ts-inferior--write-history))
-    (with-current-buffer lua-ts-inferior-buffer
-      (setq-local comint-input-ignoredups t
-                  comint-input-ring-file-name lua-ts-inferior-history
-                  comint-prompt-read-only t
-                  comint-prompt-regexp (rx-to-string `(: bol
-                                                         ,lua-ts-inferior-prompt
-                                                         (1+ space))))
-      (comint-read-input-ring t)
-      (add-hook 'comint-preoutput-filter-functions
-                (lambda (string)
-                  (if (equal string (concat lua-ts-inferior-prompt-continue " "))
-                      string
-                    (concat
-                     ;; Filter out the extra prompt characters that
-                     ;; accumulate in the output when sending regions
-                     ;; to the inferior process.
-                     (replace-regexp-in-string (rx-to-string
-                                                `(: bol
-                                                    (* ,lua-ts-inferior-prompt
-                                                       (? ,lua-ts-inferior-prompt)
-                                                       (1+ space))
-                                                    (group (* nonl))))
-                                               "\\1" string)
-                     ;; Re-add the prompt for the next line.
-                     lua-ts-inferior-prompt " ")))
-                nil t)))
-  (select-window (display-buffer lua-ts-inferior-buffer
-                                 '((display-buffer-reuse-window
-                                    display-buffer-pop-up-window)
-                                   (reusable-frames . t))))
-  (get-buffer-process (current-buffer)))
+      (with-current-buffer lua-ts-inferior-buffer
+        (setq-local comint-input-ignoredups t
+                    comint-input-ring-file-name lua-ts-inferior-history
+                    comint-prompt-read-only t
+                    comint-prompt-regexp (rx-to-string `(: bol
+                                                           ,lua-ts-inferior-prompt
+                                                           (1+ space))))
+        (comint-read-input-ring t)
+        (add-hook 'comint-preoutput-filter-functions
+                  (lambda (string)
+                    (if (equal string (concat lua-ts-inferior-prompt-continue " "))
+                        string
+                      (concat
+                       ;; Filter out the extra prompt characters that
+                       ;; accumulate in the output when sending regions
+                       ;; to the inferior process.
+                       (replace-regexp-in-string
+                        (rx-to-string `(: bol
+                                          (* ,lua-ts-inferior-prompt
+                                             (? ,lua-ts-inferior-prompt)
+                                             (1+ space))
+                                          (group (* nonl))))
+                        "\\1" string)
+                       ;; Re-add the prompt for the next line.
+                       lua-ts-inferior-prompt " ")))
+                  nil t)))
+    (select-window (display-buffer lua-ts-inferior-buffer
+                                   '((display-buffer-reuse-window
+                                      display-buffer-pop-up-window)
+                                     (reusable-frames . t))))
+    (get-buffer-process (current-buffer))))
 
 (defun lua-ts-send-buffer ()
   "Send current buffer to the inferior Lua process."
-- 
2.47.0

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[Eli Zaretskii]

> From: john muhl <jm@pub.pink>
> Cc: 74235@debbugs.gnu.org
> Date: Sat, 09 Nov 2024 14:54:47 -0600
> 
> +  (if (or (not lua-ts-inferior-program)
> +          (not (executable-find lua-ts-inferior-program)))
> +      (user-error "You must set `lua-ts-inferior-program' to use this command")

The text of user-error should say something about being unable to find
the program, not just about the variable being nil, since you use
executable-find.

Otherwise, LGTM, thanks.

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[john muhl]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: john muhl <jm@pub.pink>
>> Cc: 74235@debbugs.gnu.org
>> Date: Sat, 09 Nov 2024 14:54:47 -0600
>> 
>> +  (if (or (not lua-ts-inferior-program)
>> +          (not (executable-find lua-ts-inferior-program)))
>> +      (user-error "You must set `lua-ts-inferior-program' to use this command")
>
> The text of user-error should say something about being unable to find
> the program, not just about the variable being nil, since you use
> executable-find.
>
> Otherwise, LGTM, thanks.

I guess the executable-find check is redundant anyway since
comint/process will tell you “Searching for program: No such file
or directory, luaz” if you get it wrong.

Thanks again.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-some-lua-ts-mode-options-Bug-74235.patch --]
[-- Type: text/x-patch, Size: 5969 bytes --]

From b170e406f834806842761726de47ca3afeb05cfe Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
Date: Sat, 9 Nov 2024 11:01:45 -0600
Subject: [PATCH] ; Fix some 'lua-ts-mode' options (Bug#74235)

* lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
(lua-ts-inferior-program): Switch to 'file' type and remove 'nil'
as a choice.
(lua-ts-inferior-lua): Ensure 'lua-ts-inferior-program' is set.
---
 lisp/progmodes/lua-ts-mode.el | 84 ++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
index 4ea453c9b65..a7763377177 100644
--- a/lisp/progmodes/lua-ts-mode.el
+++ b/lisp/progmodes/lua-ts-mode.el
@@ -72,7 +72,7 @@ lua-ts-indent-offset
 
 (defcustom lua-ts-luacheck-program "luacheck"
   "Location of the Luacheck program."
-  :type '(choice (const :tag "None" nil) string)
+  :type 'file
   :version "30.1")
 
 (defcustom lua-ts-inferior-buffer "*Lua*"
@@ -83,7 +83,7 @@ lua-ts-inferior-buffer
 
 (defcustom lua-ts-inferior-program "lua"
   "Program to run in the inferior Lua process."
-  :type '(choice (const :tag "None" nil) string)
+  :type 'file
   :version "30.1")
 
 (defcustom lua-ts-inferior-options '("-i")
@@ -632,47 +632,49 @@ lua-ts-flymake-luacheck
 (defun lua-ts-inferior-lua ()
   "Run a Lua interpreter in an inferior process."
   (interactive)
-  (unless (comint-check-proc lua-ts-inferior-buffer)
-    (apply #'make-comint-in-buffer
-           (string-replace "*" "" lua-ts-inferior-buffer)
-           lua-ts-inferior-buffer
-           lua-ts-inferior-program
-           lua-ts-inferior-startfile
-           lua-ts-inferior-options)
-    (when lua-ts-inferior-history
+  (if (not lua-ts-inferior-program)
+      (user-error "You must set `lua-ts-inferior-program' to use this command")
+    (unless (comint-check-proc lua-ts-inferior-buffer)
+      (apply #'make-comint-in-buffer
+             (string-replace "*" "" lua-ts-inferior-buffer)
+             lua-ts-inferior-buffer
+             lua-ts-inferior-program
+             lua-ts-inferior-startfile
+             lua-ts-inferior-options)
+      (when lua-ts-inferior-history
         (set-process-sentinel (get-buffer-process lua-ts-inferior-buffer)
                               'lua-ts-inferior--write-history))
-    (with-current-buffer lua-ts-inferior-buffer
-      (setq-local comint-input-ignoredups t
-                  comint-input-ring-file-name lua-ts-inferior-history
-                  comint-prompt-read-only t
-                  comint-prompt-regexp (rx-to-string `(: bol
-                                                         ,lua-ts-inferior-prompt
-                                                         (1+ space))))
-      (comint-read-input-ring t)
-      (add-hook 'comint-preoutput-filter-functions
-                (lambda (string)
-                  (if (equal string (concat lua-ts-inferior-prompt-continue " "))
-                      string
-                    (concat
-                     ;; Filter out the extra prompt characters that
-                     ;; accumulate in the output when sending regions
-                     ;; to the inferior process.
-                     (replace-regexp-in-string (rx-to-string
-                                                `(: bol
-                                                    (* ,lua-ts-inferior-prompt
-                                                       (? ,lua-ts-inferior-prompt)
-                                                       (1+ space))
-                                                    (group (* nonl))))
-                                               "\\1" string)
-                     ;; Re-add the prompt for the next line.
-                     lua-ts-inferior-prompt " ")))
-                nil t)))
-  (select-window (display-buffer lua-ts-inferior-buffer
-                                 '((display-buffer-reuse-window
-                                    display-buffer-pop-up-window)
-                                   (reusable-frames . t))))
-  (get-buffer-process (current-buffer)))
+      (with-current-buffer lua-ts-inferior-buffer
+        (setq-local comint-input-ignoredups t
+                    comint-input-ring-file-name lua-ts-inferior-history
+                    comint-prompt-read-only t
+                    comint-prompt-regexp (rx-to-string `(: bol
+                                                           ,lua-ts-inferior-prompt
+                                                           (1+ space))))
+        (comint-read-input-ring t)
+        (add-hook 'comint-preoutput-filter-functions
+                  (lambda (string)
+                    (if (equal string (concat lua-ts-inferior-prompt-continue " "))
+                        string
+                      (concat
+                       ;; Filter out the extra prompt characters that
+                       ;; accumulate in the output when sending regions
+                       ;; to the inferior process.
+                       (replace-regexp-in-string
+                        (rx-to-string `(: bol
+                                          (* ,lua-ts-inferior-prompt
+                                             (? ,lua-ts-inferior-prompt)
+                                             (1+ space))
+                                          (group (* nonl))))
+                        "\\1" string)
+                       ;; Re-add the prompt for the next line.
+                       lua-ts-inferior-prompt " ")))
+                  nil t)))
+    (select-window (display-buffer lua-ts-inferior-buffer
+                                   '((display-buffer-reuse-window
+                                      display-buffer-pop-up-window)
+                                     (reusable-frames . t))))
+    (get-buffer-process (current-buffer))))
 
 (defun lua-ts-send-buffer ()
   "Send current buffer to the inferior Lua process."
-- 
2.47.0

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[Eli Zaretskii]

> From: john muhl <jm@pub.pink>
> Cc: 74235@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 08:48:39 -0600
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: john muhl <jm@pub.pink>
> >> Cc: 74235@debbugs.gnu.org
> >> Date: Sat, 09 Nov 2024 14:54:47 -0600
> >> 
> >> +  (if (or (not lua-ts-inferior-program)
> >> +          (not (executable-find lua-ts-inferior-program)))
> >> +      (user-error "You must set `lua-ts-inferior-program' to use this command")
> >
> > The text of user-error should say something about being unable to find
> > the program, not just about the variable being nil, since you use
> > executable-find.
> >
> > Otherwise, LGTM, thanks.
> 
> I guess the executable-find check is redundant anyway since
> comint/process will tell you “Searching for program: No such file
> or directory, luaz” if you get it wrong.

Thanks, installed on the emacs-30 branch, and closing the bug.

Btw, I noticed that you are marking with ";" the log entries of
changes which should definitely appear in the produced ChangeLog.  Is
this on purpose, or did you misunderstand the meaning of the
semi-colon in the first line of the commit log messages?

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[john muhl]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: john muhl <jm@pub.pink>
>> Cc: 74235@debbugs.gnu.org
>> Date: Sun, 10 Nov 2024 08:48:39 -0600
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: john muhl <jm@pub.pink>
>> >> Cc: 74235@debbugs.gnu.org
>> >> Date: Sat, 09 Nov 2024 14:54:47 -0600
>> >> 
>> >> +  (if (or (not lua-ts-inferior-program)
>> >> +          (not (executable-find lua-ts-inferior-program)))
>> >> + (user-error "You must set `lua-ts-inferior-program' to use this
>> >> command")
>> >
>> > The text of user-error should say something about being unable to find
>> > the program, not just about the variable being nil, since you use
>> > executable-find.
>> >
>> > Otherwise, LGTM, thanks.
>> 
>> I guess the executable-find check is redundant anyway since
>> comint/process will tell you “Searching for program: No such file
>> or directory, luaz” if you get it wrong.
>
> Thanks, installed on the emacs-30 branch, and closing the bug.
>
> Btw, I noticed that you are marking with ";" the log entries of
> changes which should definitely appear in the produced ChangeLog.  Is
> this on purpose, or did you misunderstand the meaning of the
> semi-colon in the first line of the commit log messages?

Temporary (hopefully) confusion. I think I had NEWS entries on my
mind and conflated the the semicolon with not needing an entry
there. I should probably re-read CONTRIBUTING since it’s been
awhile and I don’t contribute often enough to have it all
ingrained yet.

Thanks for your patience.

bug#74235: [PATCH] ; Remove 'nil' from some 'lua-ts-mode' options

[Philip Kaludercic]

john muhl <jm@pub.pink> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: john muhl <jm@pub.pink>
>>> Cc: 74235@debbugs.gnu.org
>>> Date: Fri, 08 Nov 2024 08:57:37 -0600
>>> 
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>> 
>>> >> From: john muhl <jm@pub.pink>
>>> >> Date: Wed, 06 Nov 2024 15:22:34 -0600
>>> >> 
>>> >> * lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
>>> >> (lua-ts-inferior-program): Remove 'nil' as it is never a valid
>>> >> choice.  (Bug#74235)
>>> >
>>> > IMO, the use of these options in the code is not clean: if these
>>> > programs are not installed, the commands which reference these options
>>> > will signal errors whose diagnostic might not clearly indicate the
>>> > root cause.  I would like to see a cleaner handling of such
>>> > contingencies by this mode.  Does this make sense, and if so, would
>>> > you like to suggest changes to that effect?
>>> 
>>> The only idea I’ve come up with so far is showing a warning that
>>> mentions the incorrectly set option. Does that sound like what you
>>> have in mind? If not I’d appreciate some more explicit guidance.
>>
>> How about signaling user-error from the commands that invoke those
>> programs, when they are missing?
>
> Agree that’s better than a warning. It seems flymake already
> handles the case where it can’t find the requested program so I
> left that part as is.
>
> From 3e49aa1086f2498ccbad396cf8c799c57919847e Mon Sep 17 00:00:00 2001
> From: john muhl <jm@pub.pink>
> Date: Sat, 9 Nov 2024 11:01:45 -0600
> Subject: [PATCH] ; Fix some 'lua-ts-mode' options (Bug#74235)
>
> * lisp/progmodes/lua-ts-mode.el (lua-ts-luacheck-program):
> (lua-ts-inferior-program): Switch to 'file' type and remove 'nil'
> as a choice.
> (lua-ts-inferior-lua): Ensure 'lua-ts-inferior-program' is a
> valid executable.
> ---
>  lisp/progmodes/lua-ts-mode.el | 85 ++++++++++++++++++-----------------
>  1 file changed, 44 insertions(+), 41 deletions(-)
>
> diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
> index 4ea453c9b65..2323461ed4c 100644
> --- a/lisp/progmodes/lua-ts-mode.el
> +++ b/lisp/progmodes/lua-ts-mode.el
> @@ -72,7 +72,7 @@ lua-ts-indent-offset
>  
>  (defcustom lua-ts-luacheck-program "luacheck"
>    "Location of the Luacheck program."
> -  :type '(choice (const :tag "None" nil) string)
> +  :type 'file
>    :version "30.1")
>  
>  (defcustom lua-ts-inferior-buffer "*Lua*"
> @@ -83,7 +83,7 @@ lua-ts-inferior-buffer
>  
>  (defcustom lua-ts-inferior-program "lua"
>    "Program to run in the inferior Lua process."
> -  :type '(choice (const :tag "None" nil) string)
> +  :type 'file
>    :version "30.1")
>  
>  (defcustom lua-ts-inferior-options '("-i")
> @@ -632,47 +632,50 @@ lua-ts-flymake-luacheck
>  (defun lua-ts-inferior-lua ()
>    "Run a Lua interpreter in an inferior process."
>    (interactive)
> -  (unless (comint-check-proc lua-ts-inferior-buffer)
> -    (apply #'make-comint-in-buffer
> -           (string-replace "*" "" lua-ts-inferior-buffer)
> -           lua-ts-inferior-buffer
> -           lua-ts-inferior-program
> -           lua-ts-inferior-startfile
> -           lua-ts-inferior-options)
> -    (when lua-ts-inferior-history
> +  (if (or (not lua-ts-inferior-program)
> +          (not (executable-find lua-ts-inferior-program)))
> +      (user-error "You must set `lua-ts-inferior-program' to use this command")

Isn't this error message misleading as `lua-ts-inferior-program' can be
set (as it is by default), but the executable couldn't be located (which
isn't a mistake by the user, but just due to Lua not being installed)?

> +    (unless (comint-check-proc lua-ts-inferior-buffer)
> +      (apply #'make-comint-in-buffer
> +             (string-replace "*" "" lua-ts-inferior-buffer)
> +             lua-ts-inferior-buffer
> +             lua-ts-inferior-program
> +             lua-ts-inferior-startfile
> +             lua-ts-inferior-options)
> +      (when lua-ts-inferior-history
>          (set-process-sentinel (get-buffer-process lua-ts-inferior-buffer)
>                                'lua-ts-inferior--write-history))
> -    (with-current-buffer lua-ts-inferior-buffer
> -      (setq-local comint-input-ignoredups t
> -                  comint-input-ring-file-name lua-ts-inferior-history
> -                  comint-prompt-read-only t
> -                  comint-prompt-regexp (rx-to-string `(: bol
> -                                                         ,lua-ts-inferior-prompt
> -                                                         (1+ space))))
> -      (comint-read-input-ring t)
> -      (add-hook 'comint-preoutput-filter-functions
> -                (lambda (string)
> -                  (if (equal string (concat lua-ts-inferior-prompt-continue " "))
> -                      string
> -                    (concat
> -                     ;; Filter out the extra prompt characters that
> -                     ;; accumulate in the output when sending regions
> -                     ;; to the inferior process.
> -                     (replace-regexp-in-string (rx-to-string
> -                                                `(: bol
> -                                                    (* ,lua-ts-inferior-prompt
> -                                                       (? ,lua-ts-inferior-prompt)
> -                                                       (1+ space))
> -                                                    (group (* nonl))))
> -                                               "\\1" string)
> -                     ;; Re-add the prompt for the next line.
> -                     lua-ts-inferior-prompt " ")))
> -                nil t)))
> -  (select-window (display-buffer lua-ts-inferior-buffer
> -                                 '((display-buffer-reuse-window
> -                                    display-buffer-pop-up-window)
> -                                   (reusable-frames . t))))
> -  (get-buffer-process (current-buffer)))
> +      (with-current-buffer lua-ts-inferior-buffer
> +        (setq-local comint-input-ignoredups t
> +                    comint-input-ring-file-name lua-ts-inferior-history
> +                    comint-prompt-read-only t
> +                    comint-prompt-regexp (rx-to-string `(: bol
> +                                                           ,lua-ts-inferior-prompt
> +                                                           (1+ space))))

This should be equivalent to

  (rx bol (literal lua-ts-inferior-prompt) (1+ space))

that macroexpands to

  (concat "^" (regexp-quote inferior-lisp-program) "[[:space:]]+")

without the need to load rx at run-time.

> +        (comint-read-input-ring t)
> +        (add-hook 'comint-preoutput-filter-functions
> +                  (lambda (string)
> +                    (if (equal string (concat lua-ts-inferior-prompt-continue " "))
> +                        string
> +                      (concat
> +                       ;; Filter out the extra prompt characters that
> +                       ;; accumulate in the output when sending regions
> +                       ;; to the inferior process.
> +                       (replace-regexp-in-string
> +                        (rx-to-string `(: bol
> +                                          (* ,lua-ts-inferior-prompt
> +                                             (? ,lua-ts-inferior-prompt)
> +                                             (1+ space))
> +                                          (group (* nonl))))
> +                        "\\1" string)
> +                       ;; Re-add the prompt for the next line.
> +                       lua-ts-inferior-prompt " ")))
> +                  nil t)))
> +    (select-window (display-buffer lua-ts-inferior-buffer
> +                                   '((display-buffer-reuse-window
> +                                      display-buffer-pop-up-window)
> +                                     (reusable-frames . t))))
> +    (get-buffer-process (current-buffer))))
>  
>  (defun lua-ts-send-buffer ()
>    "Send current buffer to the inferior Lua process."

-- 
	Philip Kaludercic on siskin