* bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
@ 2024-04-03 17:55 john muhl
[not found] ` <handler.70167.B.171216697516444.ack@debbugs.gnu.org>
0 siblings, 1 reply; 7+ messages in thread
From: john muhl @ 2024-04-03 17:55 UTC (permalink / raw)
To: 70167
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
[not found] ` <handler.70167.B.171216697516444.ack@debbugs.gnu.org>
@ 2024-04-03 17:59 ` john muhl
2024-04-04 6:05 ` Philip Kaludercic
2024-04-04 6:05 ` Philip Kaludercic
0 siblings, 2 replies; 7+ messages in thread
From: john muhl @ 2024-04-03 17:59 UTC (permalink / raw)
To: 70167
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Mark-Flymake-regions-more-accurately-in-lua-ts-mode.patch --]
[-- Type: text/x-patch, Size: 4634 bytes --]
From a0c1f9c84a7072a807141f7b304a3c98c8e92173 Mon Sep 17 00:00:00 2001
From: john muhl <jm@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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
2024-04-03 17:59 ` john muhl
@ 2024-04-04 6:05 ` Philip Kaludercic
2024-04-04 6:05 ` Philip Kaludercic
1 sibling, 0 replies; 7+ messages in thread
From: Philip Kaludercic @ 2024-04-04 6:05 UTC (permalink / raw)
To: john muhl; +Cc: 70167
john muhl <jm@pub.pink> writes:
>>From a0c1f9c84a7072a807141f7b304a3c98c8e92173 Mon Sep 17 00:00:00 2001
> From: john muhl <jm@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
2024-04-03 17:59 ` john muhl
2024-04-04 6:05 ` Philip Kaludercic
@ 2024-04-04 6:05 ` Philip Kaludercic
2024-04-04 16:45 ` john muhl
1 sibling, 1 reply; 7+ messages in thread
From: Philip Kaludercic @ 2024-04-04 6:05 UTC (permalink / raw)
To: john muhl; +Cc: 70167
john muhl <jm@pub.pink> writes:
>>From a0c1f9c84a7072a807141f7b304a3c98c8e92173 Mon Sep 17 00:00:00 2001
> From: john muhl <jm@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
2024-04-04 6:05 ` Philip Kaludercic
@ 2024-04-04 16:45 ` john muhl
2024-04-10 8:28 ` Philip Kaludercic
2024-04-13 8:12 ` Eli Zaretskii
0 siblings, 2 replies; 7+ messages in thread
From: john muhl @ 2024-04-04 16:45 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 70167
[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> john muhl <jm@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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Mark-Flymake-regions-more-accurately-in-lua-ts-mode.patch --]
[-- Type: text/x-patch, Size: 4526 bytes --]
From 910e88cf83415ae1e077fbc36560cb29fd39b666 Mon Sep 17 00:00:00 2001
From: john muhl <jm@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))
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
2024-04-04 16:45 ` john muhl
@ 2024-04-10 8:28 ` Philip Kaludercic
2024-04-13 8:12 ` Eli Zaretskii
1 sibling, 0 replies; 7+ messages in thread
From: Philip Kaludercic @ 2024-04-10 8:28 UTC (permalink / raw)
To: john muhl; +Cc: 70167
john muhl <jm@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@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode
2024-04-04 16:45 ` john muhl
2024-04-10 8:28 ` Philip Kaludercic
@ 2024-04-13 8:12 ` Eli Zaretskii
1 sibling, 0 replies; 7+ messages in thread
From: Eli Zaretskii @ 2024-04-13 8:12 UTC (permalink / raw)
To: john muhl; +Cc: philipk, 70167-done
> Cc: 70167@debbugs.gnu.org
> From: john muhl <jm@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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-13 8:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-03 17:55 bug#70167: [PATCH] Mark Flymake regions more accurately in lua-ts-mode john muhl
[not found] ` <handler.70167.B.171216697516444.ack@debbugs.gnu.org>
2024-04-03 17:59 ` john muhl
2024-04-04 6:05 ` Philip Kaludercic
2024-04-04 6:05 ` Philip Kaludercic
2024-04-04 16:45 ` john muhl
2024-04-10 8:28 ` Philip Kaludercic
2024-04-13 8:12 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).