Package: emacs;
Reported by: john muhl <jm <at> pub.pink>
Date: Wed, 3 Apr 2024 17:57:04 UTC
Severity: normal
Tags: patch
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 70167 in the body.
You can then email your comments to 70167 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
bug-gnu-emacs <at> gnu.org:bug#70167; Package emacs.
(Wed, 03 Apr 2024 17:57:05 GMT) Full text and rfc822 format available.john muhl <jm <at> pub.pink>:bug-gnu-emacs <at> gnu.org.
(Wed, 03 Apr 2024 17:57:05 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: john muhl <jm <at> pub.pink> To: bug-gnu-emacs <at> gnu.org Subject: [PATCH] Mark Flymake regions more accurately in lua-ts-mode Date: Wed, 03 Apr 2024 12:55:33 -0500
Tags: patch This changes the diagnostic region end position from being guessed based on thing-at-point to using the one provided by LuaCheck. As noted in bug#67152 it was already being matched but not used. I couldn’t find anything where t-a-p guessed wrong but it seems preferable to just mark exactly the same region as LuaCheck would.
bug-gnu-emacs <at> gnu.org:bug#70167; Package emacs.
(Wed, 03 Apr 2024 18:09:02 GMT) Full text and rfc822 format available.Message #8 received at 70167 <at> debbugs.gnu.org (full text, mbox):
From: john muhl <jm <at> pub.pink> To: 70167 <at> debbugs.gnu.org Subject: Re: bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode Date: Wed, 03 Apr 2024 12:59:24 -0500
[0001-Mark-Flymake-regions-more-accurately-in-lua-ts-mode.patch (text/x-patch, attachment)]
From a0c1f9c84a7072a807141f7b304a3c98c8e92173 Mon Sep 17 00:00:00 2001
From: john muhl <jm <at> pub.pink>
Date: Wed, 13 Mar 2024 08:35:08 -0500
Subject: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
* lisp/progmodes/lua-ts-mode.el (lua-ts-flymake-luacheck): Use
the end position provided by Luacheck rather than relying on
'thing-at-point' to guess where the end should be. (bug#70167)
---
lisp/progmodes/lua-ts-mode.el | 55 ++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 27 deletions(-)
diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
index 407ef230c32..a39cfa9e814 100644
--- a/lisp/progmodes/lua-ts-mode.el
+++ b/lisp/progmodes/lua-ts-mode.el
@@ -35,7 +35,6 @@
(require 'treesit)
(eval-when-compile
- (require 'cl-lib)
(require 'rx))
(declare-function treesit-induce-sparse-tree "treesit.c")
@@ -544,32 +543,34 @@ lua-ts-flymake-luacheck
(eq proc lua-ts--flymake-process))
(with-current-buffer (process-buffer proc)
(goto-char (point-min))
- (cl-loop
- while (search-forward-regexp
- (rx (seq bol
- (0+ alnum) ":"
- (group (1+ digit)) ":"
- (group (1+ digit)) "-"
- (group (1+ digit)) ": "
- (group (0+ nonl))
- eol))
- nil t)
- for (beg . end) = (flymake-diag-region
- source
- (string-to-number (match-string 1))
- (string-to-number (match-string 2)))
- for msg = (match-string 4)
- for type = (if (string-match "^(W" msg)
- :warning
- :error)
- when (and beg end)
- collect (flymake-make-diagnostic source
- beg
- end
- type
- msg)
- into diags
- finally (funcall report-fn diags)))
+ (let (beg end msg type diags)
+ (while
+ (search-forward-regexp
+ (rx (: bol (0+ alnum) ":"
+ (group (1+ digit)) ":"
+ (group (1+ digit)) "-"
+ (group (1+ digit)) ": "
+ (group (0+ nonl)) eol))
+ nil t)
+ (setq beg
+ (car (flymake-diag-region
+ source
+ (string-to-number (match-string 1))
+ (string-to-number (match-string 2)))))
+ (setq end
+ (cdr (flymake-diag-region
+ source
+ (string-to-number (match-string 1))
+ (string-to-number (match-string 3)))))
+ (setq msg (match-string 4))
+ (setq type (if (string-match "^(W" msg) :warning
+ :error))
+ (when (and beg end)
+ (setq diags
+ (nconc diags
+ (list (flymake-make-diagnostic
+ source beg end type msg))))))
+ (funcall report-fn diags)))
(flymake-log :warning "Canceling obsolete check %s" proc))
(kill-buffer (process-buffer proc)))))))
(process-send-region lua-ts--flymake-process (point-min) (point-max))
--
2.41.0
bug-gnu-emacs <at> gnu.org:bug#70167; Package emacs.
(Thu, 04 Apr 2024 06:06:02 GMT) Full text and rfc822 format available.Message #11 received at 70167 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: john muhl <jm <at> pub.pink> Cc: 70167 <at> debbugs.gnu.org Subject: Re: bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode Date: Thu, 04 Apr 2024 06:05:03 +0000
john muhl <jm <at> pub.pink> writes:
>>From a0c1f9c84a7072a807141f7b304a3c98c8e92173 Mon Sep 17 00:00:00 2001
> From: john muhl <jm <at> pub.pink>
> Date: Wed, 13 Mar 2024 08:35:08 -0500
> Subject: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
>
> * lisp/progmodes/lua-ts-mode.el (lua-ts-flymake-luacheck): Use
> the end position provided by Luacheck rather than relying on
> 'thing-at-point' to guess where the end should be. (bug#70167)
> ---
> lisp/progmodes/lua-ts-mode.el | 55 ++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 27 deletions(-)
>
> diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
> index 407ef230c32..a39cfa9e814 100644
> --- a/lisp/progmodes/lua-ts-mode.el
> +++ b/lisp/progmodes/lua-ts-mode.el
> @@ -35,7 +35,6 @@
> (require 'treesit)
>
> (eval-when-compile
> - (require 'cl-lib)
> (require 'rx))
>
> (declare-function treesit-induce-sparse-tree "treesit.c")
> @@ -544,32 +543,34 @@ lua-ts-flymake-luacheck
> (eq proc lua-ts--flymake-process))
> (with-current-buffer (process-buffer proc)
> (goto-char (point-min))
> - (cl-loop
> - while (search-forward-regexp
> - (rx (seq bol
> - (0+ alnum) ":"
> - (group (1+ digit)) ":"
> - (group (1+ digit)) "-"
> - (group (1+ digit)) ": "
> - (group (0+ nonl))
> - eol))
> - nil t)
> - for (beg . end) = (flymake-diag-region
> - source
> - (string-to-number (match-string 1))
> - (string-to-number (match-string 2)))
> - for msg = (match-string 4)
> - for type = (if (string-match "^(W" msg)
> - :warning
> - :error)
> - when (and beg end)
> - collect (flymake-make-diagnostic source
> - beg
> - end
> - type
> - msg)
> - into diags
> - finally (funcall report-fn diags)))
> + (let (beg end msg type diags)
> + (while
Why do you declare these variables outside of the loop? Should the
values persist between iterations? If not, you could avoid the setq
soup below, by declaring and binding the variables at once.
> + (search-forward-regexp
> + (rx (: bol (0+ alnum) ":"
^
this is not necessary, since
the rx body has an implicit ":".
> + (group (1+ digit)) ":"
> + (group (1+ digit)) "-"
> + (group (1+ digit)) ": "
> + (group (0+ nonl)) eol))
> + nil t)
> + (setq beg
> + (car (flymake-diag-region
> + source
> + (string-to-number (match-string 1))
> + (string-to-number (match-string 2)))))
> + (setq end
> + (cdr (flymake-diag-region
> + source
> + (string-to-number (match-string 1))
> + (string-to-number (match-string 3)))))
> + (setq msg (match-string 4))
> + (setq type (if (string-match "^(W" msg) :warning
^
You can avoid a
regular expression
here using `string-prefix-p'.
> + :error))
> + (when (and beg end)
> + (setq diags
> + (nconc diags
> + (list (flymake-make-diagnostic
> + source beg end type msg))))))
> + (funcall report-fn diags)))
If I see this correctly, then you are appending each element to the end
of the list? If so, it would be more efficient to just construct the
list in reverse using `push' and then `nreverse'ing it before passing it
to REPORT-FN.
> (flymake-log :warning "Canceling obsolete check %s" proc))
> (kill-buffer (process-buffer proc)))))))
> (process-send-region lua-ts--flymake-process (point-min) (point-max))
--
Philip Kaludercic on peregrine
bug-gnu-emacs <at> gnu.org:bug#70167; Package emacs.
(Thu, 04 Apr 2024 06:07:01 GMT) Full text and rfc822 format available.Message #14 received at 70167 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: john muhl <jm <at> pub.pink> Cc: 70167 <at> debbugs.gnu.org Subject: Re: bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode Date: Thu, 04 Apr 2024 06:05:49 +0000
john muhl <jm <at> pub.pink> writes:
>>From a0c1f9c84a7072a807141f7b304a3c98c8e92173 Mon Sep 17 00:00:00 2001
> From: john muhl <jm <at> pub.pink>
> Date: Wed, 13 Mar 2024 08:35:08 -0500
> Subject: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
>
> * lisp/progmodes/lua-ts-mode.el (lua-ts-flymake-luacheck): Use
> the end position provided by Luacheck rather than relying on
> 'thing-at-point' to guess where the end should be. (bug#70167)
> ---
> lisp/progmodes/lua-ts-mode.el | 55 ++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 27 deletions(-)
>
> diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
> index 407ef230c32..a39cfa9e814 100644
> --- a/lisp/progmodes/lua-ts-mode.el
> +++ b/lisp/progmodes/lua-ts-mode.el
> @@ -35,7 +35,6 @@
> (require 'treesit)
>
> (eval-when-compile
> - (require 'cl-lib)
> (require 'rx))
>
> (declare-function treesit-induce-sparse-tree "treesit.c")
> @@ -544,32 +543,34 @@ lua-ts-flymake-luacheck
> (eq proc lua-ts--flymake-process))
> (with-current-buffer (process-buffer proc)
> (goto-char (point-min))
> - (cl-loop
> - while (search-forward-regexp
> - (rx (seq bol
> - (0+ alnum) ":"
> - (group (1+ digit)) ":"
> - (group (1+ digit)) "-"
> - (group (1+ digit)) ": "
> - (group (0+ nonl))
> - eol))
> - nil t)
> - for (beg . end) = (flymake-diag-region
> - source
> - (string-to-number (match-string 1))
> - (string-to-number (match-string 2)))
> - for msg = (match-string 4)
> - for type = (if (string-match "^(W" msg)
> - :warning
> - :error)
> - when (and beg end)
> - collect (flymake-make-diagnostic source
> - beg
> - end
> - type
> - msg)
> - into diags
> - finally (funcall report-fn diags)))
> + (let (beg end msg type diags)
> + (while
Why do you declare these variables outside of the loop? Should the
values persist between iterations? If not, you could avoid the setq
soup below, by declaring and binding the variables at once.
> + (search-forward-regexp
> + (rx (: bol (0+ alnum) ":"
^
this is not necessary, since
the rx body has an implicit ":".
> + (group (1+ digit)) ":"
> + (group (1+ digit)) "-"
> + (group (1+ digit)) ": "
> + (group (0+ nonl)) eol))
> + nil t)
> + (setq beg
> + (car (flymake-diag-region
> + source
> + (string-to-number (match-string 1))
> + (string-to-number (match-string 2)))))
> + (setq end
> + (cdr (flymake-diag-region
> + source
> + (string-to-number (match-string 1))
> + (string-to-number (match-string 3)))))
> + (setq msg (match-string 4))
> + (setq type (if (string-match "^(W" msg) :warning
^
You can avoid a
regular expression
here using `string-prefix-p'.
> + :error))
> + (when (and beg end)
> + (setq diags
> + (nconc diags
> + (list (flymake-make-diagnostic
> + source beg end type msg))))))
> + (funcall report-fn diags)))
If I see this correctly, then you are appending each element to the end
of the list? If so, it would be more efficient to just construct the
list in reverse using `push' and then `nreverse'ing it before passing it
to REPORT-FN.
> (flymake-log :warning "Canceling obsolete check %s" proc))
> (kill-buffer (process-buffer proc)))))))
> (process-send-region lua-ts--flymake-process (point-min) (point-max))
--
Philip Kaludercic on peregrine
bug-gnu-emacs <at> gnu.org:bug#70167; Package emacs.
(Thu, 04 Apr 2024 19:40:04 GMT) Full text and rfc822 format available.Message #17 received at 70167 <at> debbugs.gnu.org (full text, mbox):
From: john muhl <jm <at> pub.pink> To: Philip Kaludercic <philipk <at> posteo.net> Cc: 70167 <at> debbugs.gnu.org Subject: Re: bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode Date: Thu, 04 Apr 2024 11:45:05 -0500
[Message part 1 (text/plain, inline)]
Philip Kaludercic <philipk <at> posteo.net> writes: > john muhl <jm <at> pub.pink> writes: > >> + (let (beg end msg type diags) >> + (while > > Why do you declare these variables outside of the loop? Should the > values persist between iterations? If not, you could avoid the setq > soup below, by declaring and binding the variables at once. Only the list of diagnostics is used outside the loop. I’ve moved the others inside the while. >> + (search-forward-regexp >> + (rx (: bol (0+ alnum) ":" > ^ > this is not necessary, since > the rx body has an implicit ":". Fixed. >> + (setq msg (match-string 4)) >> + (setq type (if (string-match "^(W" msg) :warning > ^ > You can avoid a > regular expression > here using `string-prefix-p'. Fixed. >> + :error)) >> + (when (and beg end) >> + (setq diags >> + (nconc diags >> + (list (flymake-make-diagnostic >> + source beg end type msg)))))) >> + (funcall report-fn diags))) > > If I see this correctly, then you are appending each element to the end > of the list? If so, it would be more efficient to just construct the > list in reverse using `push' and then `nreverse'ing it before passing it > to REPORT-FN. Changed to use push. It doesn’t look like the order matters or did I misunderstand something? Thanks for the review.
[0001-Mark-Flymake-regions-more-accurately-in-lua-ts-mode.patch (text/x-patch, attachment)]
bug-gnu-emacs <at> gnu.org:bug#70167; Package emacs.
(Wed, 10 Apr 2024 08:29:03 GMT) Full text and rfc822 format available.Message #20 received at 70167 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: john muhl <jm <at> pub.pink> Cc: 70167 <at> debbugs.gnu.org Subject: Re: bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode Date: Wed, 10 Apr 2024 08:28:08 +0000
john muhl <jm <at> pub.pink> writes: >>> + :error)) >>> + (when (and beg end) >>> + (setq diags >>> + (nconc diags >>> + (list (flymake-make-diagnostic >>> + source beg end type msg)))))) >>> + (funcall report-fn diags))) >> >> If I see this correctly, then you are appending each element to the end >> of the list? If so, it would be more efficient to just construct the >> list in reverse using `push' and then `nreverse'ing it before passing it >> to REPORT-FN. > > Changed to use push. It doesn’t look like the order matters or did > I misunderstand something? It would make sense that it shouldn't matter (unless there is some subtle performance penalty in traversing the buffer in some order), so I think you can drop the nreverse. > Thanks for the review. > >>From 910e88cf83415ae1e077fbc36560cb29fd39b666 Mon Sep 17 00:00:00 2001 > From: john muhl <jm <at> pub.pink> > Date: Wed, 13 Mar 2024 08:35:08 -0500 > Subject: [PATCH] Mark Flymake regions more accurately in lua-ts-mode > > * lisp/progmodes/lua-ts-mode.el (lua-ts-flymake-luacheck): Use > the end position provided by Luacheck rather than relying on > 'thing-at-point' to guess where the end should be. (bug#70167) > --- > lisp/progmodes/lua-ts-mode.el | 53 +++++++++++++++++------------------ > 1 file changed, 26 insertions(+), 27 deletions(-) > > diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el > index 407ef230c32..45ea8ec9a81 100644 > --- a/lisp/progmodes/lua-ts-mode.el > +++ b/lisp/progmodes/lua-ts-mode.el > @@ -35,7 +35,6 @@ > (require 'treesit) > > (eval-when-compile > - (require 'cl-lib) > (require 'rx)) > > (declare-function treesit-induce-sparse-tree "treesit.c") > @@ -544,32 +543,32 @@ lua-ts-flymake-luacheck > (eq proc lua-ts--flymake-process)) > (with-current-buffer (process-buffer proc) > (goto-char (point-min)) > - (cl-loop > - while (search-forward-regexp > - (rx (seq bol > - (0+ alnum) ":" > - (group (1+ digit)) ":" > - (group (1+ digit)) "-" > - (group (1+ digit)) ": " > - (group (0+ nonl)) > - eol)) > - nil t) > - for (beg . end) = (flymake-diag-region > - source > - (string-to-number (match-string 1)) > - (string-to-number (match-string 2))) > - for msg = (match-string 4) > - for type = (if (string-match "^(W" msg) > - :warning > - :error) > - when (and beg end) > - collect (flymake-make-diagnostic source > - beg > - end > - type > - msg) > - into diags > - finally (funcall report-fn diags))) > + (let (diags) > + (while (search-forward-regexp > + (rx bol (0+ alnum) ":" > + (group (1+ digit)) ":" > + (group (1+ digit)) "-" > + (group (1+ digit)) ": " > + (group (0+ nonl)) eol) > + nil t) > + (let* ((beg > + (car (flymake-diag-region > + source > + (string-to-number (match-string 1)) > + (string-to-number (match-string 2))))) > + (end > + (cdr (flymake-diag-region > + source > + (string-to-number (match-string 1)) > + (string-to-number (match-string 3))))) > + (msg (match-string 4)) > + (type (if (string-prefix-p "(W" msg) > + :warning > + :error))) > + (push (flymake-make-diagnostic > + source beg end type msg) > + diags))) > + (funcall report-fn diags))) > (flymake-log :warning "Canceling obsolete check %s" proc)) > (kill-buffer (process-buffer proc))))))) > (process-send-region lua-ts--flymake-process (point-min) (point-max)) LGTM, but I am not really a Flymake expert. -- Philip Kaludercic on peregrine
Eli Zaretskii <eliz <at> gnu.org>:john muhl <jm <at> pub.pink>:Message #25 received at 70167-done <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: john muhl <jm <at> pub.pink> Cc: philipk <at> posteo.net, 70167-done <at> debbugs.gnu.org Subject: Re: bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode Date: Sat, 13 Apr 2024 11:12:14 +0300
> Cc: 70167 <at> debbugs.gnu.org > From: john muhl <jm <at> pub.pink> > Date: Thu, 04 Apr 2024 11:45:05 -0500 > > > Changed to use push. It doesn’t look like the order matters or did > I misunderstand something? > > Thanks for the review. Thanks, I installed this on the master branch, and I'm closing the bug.
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Sat, 11 May 2024 11:24:08 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.