* 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[parent not found: <handler.70167.B.171216697516444.ack@debbugs.gnu.org>]
* 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).