unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
@ 2024-06-12  8:43 Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-06-27  7:32 ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-06-12  8:43 UTC (permalink / raw)
  To: 71504

Following a recent short discussion[0] on emacs-devel, I'm opening this
feature request so it doesn't get lost:

Flymake should provide a standard way for backends to associate fix
suggestions with the diagnostics they produce, along with a
backend-agnostic user interface (e.g. a command) for examining and
applying such fixes.

I suggested a possible implementation that works quite well for me
(with the three backends I've adapted so far - checkdoc, shellcheck
and Eglot), but any other solution that gives various backends a
standard way to provide their fixes would be just as welcome.


Thanks and best regards,

Eshel


[0] https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01137.html





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-06-12  8:43 bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-06-27  7:32 ` Eli Zaretskii
  2024-06-27 13:35   ` Spencer Baugh
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-06-27  7:32 UTC (permalink / raw)
  To: Eshel Yaron, Spencer Baugh; +Cc: 71504

> Date: Wed, 12 Jun 2024 10:43:14 +0200
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Following a recent short discussion[0] on emacs-devel, I'm opening this
> feature request so it doesn't get lost:
> 
> Flymake should provide a standard way for backends to associate fix
> suggestions with the diagnostics they produce, along with a
> backend-agnostic user interface (e.g. a command) for examining and
> applying such fixes.
> 
> I suggested a possible implementation that works quite well for me
> (with the three backends I've adapted so far - checkdoc, shellcheck
> and Eglot), but any other solution that gives various backends a
> standard way to provide their fixes would be just as welcome.
> 
> 
> Thanks and best regards,
> 
> Eshel
> 
> 
> [0] https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01137.html

Spencer, any comments or suggestions?

Thanks.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-06-27  7:32 ` Eli Zaretskii
@ 2024-06-27 13:35   ` Spencer Baugh
  2024-06-27 18:15     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 22+ messages in thread
From: Spencer Baugh @ 2024-06-27 13:35 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 71504, Eshel Yaron

Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Wed, 12 Jun 2024 10:43:14 +0200
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Following a recent short discussion[0] on emacs-devel, I'm opening this
>> feature request so it doesn't get lost:
>> 
>> Flymake should provide a standard way for backends to associate fix
>> suggestions with the diagnostics they produce, along with a
>> backend-agnostic user interface (e.g. a command) for examining and
>> applying such fixes.
>> 
>> I suggested a possible implementation that works quite well for me
>> (with the three backends I've adapted so far - checkdoc, shellcheck
>> and Eglot), but any other solution that gives various backends a
>> standard way to provide their fixes would be just as welcome.
>> 
>> 
>> Thanks and best regards,
>> 
>> Eshel
>> 
>> 
>> [0] https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01137.html
>
> Spencer, any comments or suggestions?
>
> Thanks.

I basically agree with Joao's views in the linked thread.  It is already
possible to associate arbitrary data and actions with flymake
diagnostics by adding overlay properties, as Joao mentioned.

I also think we have not sufficiently explored different UI
possibilities, and I think adding to flymake now will limit us in what
UI possibilities we can explore, no matter how generic we try to be.
And that UI exploration can happen without adding to flymake.

For example, maybe we want to have a command which can accept fixes
output by a process running in M-x compile.  Baking the UI into flymake
would make that impossible, wouldn't it?

So before any change in flymake I would like to see much more
exploration of "fix" UIs which are genuinely flymake-independent.  And
fit in well with existing Emacs functionality, rather than just porting
VSCode and LSP patterns.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-06-27 13:35   ` Spencer Baugh
@ 2024-06-27 18:15     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-06  7:50       ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-06-27 18:15 UTC (permalink / raw)
  To: Spencer Baugh; +Cc: Eli Zaretskii, 71504

Hi Spencer,

Thanks for taking a look.

Spencer Baugh <sbaugh@janestreet.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>>> Date: Wed, 12 Jun 2024 10:43:14 +0200
>>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>> 
>>> Following a recent short discussion[0] on emacs-devel, I'm opening this
>>> feature request so it doesn't get lost:
>>> 
>>> Flymake should provide a standard way for backends to associate fix
>>> suggestions with the diagnostics they produce, along with a
>>> backend-agnostic user interface (e.g. a command) for examining and
>>> applying such fixes.
>>> 
>>> I suggested a possible implementation that works quite well for me
>>> (with the three backends I've adapted so far - checkdoc, shellcheck
>>> and Eglot), but any other solution that gives various backends a
>>> standard way to provide their fixes would be just as welcome.

[...]

> I also think we have not sufficiently explored different UI
> possibilities, and I think adding to flymake now will limit us in what
> UI possibilities we can explore, no matter how generic we try to be.
> And that UI exploration can happen without adding to flymake.
>
> For example, maybe we want to have a command which can accept fixes
> output by a process running in M-x compile.  Baking the UI into flymake
> would make that impossible, wouldn't it?

I don't think adding a command for fixing the diagnostic at point should
preclude any other developments or explorations.  It's a useful thing to
have, and many Flymake backends have the needed data readily available.

> So before any change in flymake I would like to see much more
> exploration of "fix" UIs which are genuinely flymake-independent.

Flymake shows diagnostics, and "fixing" is what we do to diagnostics.
What would be the benefit of a Flymake-independent UI for fixing the
diagnostics that Flymake already shows?


Eshel





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-06-27 18:15     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-07-06  7:50       ` Eli Zaretskii
  2024-07-07  8:53         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-07-06  7:50 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: sbaugh, 71504

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  71504@debbugs.gnu.org
> Date: Thu, 27 Jun 2024 20:15:37 +0200
> 
> Spencer Baugh <sbaugh@janestreet.com> writes:
> 
> > Eli Zaretskii <eliz@gnu.org> writes:
> >>> Date: Wed, 12 Jun 2024 10:43:14 +0200
> >>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
> >>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >>> 
> >>> Following a recent short discussion[0] on emacs-devel, I'm opening this
> >>> feature request so it doesn't get lost:
> >>> 
> >>> Flymake should provide a standard way for backends to associate fix
> >>> suggestions with the diagnostics they produce, along with a
> >>> backend-agnostic user interface (e.g. a command) for examining and
> >>> applying such fixes.
> >>> 
> >>> I suggested a possible implementation that works quite well for me
> >>> (with the three backends I've adapted so far - checkdoc, shellcheck
> >>> and Eglot), but any other solution that gives various backends a
> >>> standard way to provide their fixes would be just as welcome.
> 
> [...]
> 
> > I also think we have not sufficiently explored different UI
> > possibilities, and I think adding to flymake now will limit us in what
> > UI possibilities we can explore, no matter how generic we try to be.
> > And that UI exploration can happen without adding to flymake.
> >
> > For example, maybe we want to have a command which can accept fixes
> > output by a process running in M-x compile.  Baking the UI into flymake
> > would make that impossible, wouldn't it?
> 
> I don't think adding a command for fixing the diagnostic at point should
> preclude any other developments or explorations.  It's a useful thing to
> have, and many Flymake backends have the needed data readily available.
> 
> > So before any change in flymake I would like to see much more
> > exploration of "fix" UIs which are genuinely flymake-independent.
> 
> Flymake shows diagnostics, and "fixing" is what we do to diagnostics.
> What would be the benefit of a Flymake-independent UI for fixing the
> diagnostics that Flymake already shows?

The benefit would be that we will be able to use that UI when "fixes"
are shown in, for example, the *compilation* buffer.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-06  7:50       ` Eli Zaretskii
@ 2024-07-07  8:53         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-07 10:44           ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-07-07  8:53 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: sbaugh, 71504

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  71504@debbugs.gnu.org
>> Date: Thu, 27 Jun 2024 20:15:37 +0200
>> 
>> Spencer Baugh <sbaugh@janestreet.com> writes:
>>
>> > For example, maybe we want to have a command which can accept fixes
>> > output by a process running in M-x compile.  Baking the UI into flymake
>> > would make that impossible, wouldn't it?
>> 
>> I don't think adding a command for fixing the diagnostic at point should
>> preclude any other developments or explorations.  It's a useful thing to
>> have, and many Flymake backends have the needed data readily available.
>> 
>> > So before any change in flymake I would like to see much more
>> > exploration of "fix" UIs which are genuinely flymake-independent.
>> 
>> Flymake shows diagnostics, and "fixing" is what we do to diagnostics.
>> What would be the benefit of a Flymake-independent UI for fixing the
>> diagnostics that Flymake already shows?
>
> The benefit would be that we will be able to use that UI when "fixes"
> are shown in, for example, the *compilation* buffer.

It'd be good to enhance compilation buffers as well, but this feature
request is about interaction with Flymake diagnostics, that are shown in
the diagnosed buffer: I'd like to have a standard way to act on (fix)
the diagnostic at point.


Thanks,

Eshel







^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-07  8:53         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-07-07 10:44           ` Eli Zaretskii
  2024-07-07 11:50             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-07-07 10:44 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: sbaugh, 71504

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
> Date: Sun, 07 Jul 2024 10:53:12 +0200
> 
> >> From: Eshel Yaron <me@eshelyaron.com>
> >> Cc: Eli Zaretskii <eliz@gnu.org>,  71504@debbugs.gnu.org
> >> Date: Thu, 27 Jun 2024 20:15:37 +0200
> >> 
> >> Spencer Baugh <sbaugh@janestreet.com> writes:
> >>
> >> > For example, maybe we want to have a command which can accept fixes
> >> > output by a process running in M-x compile.  Baking the UI into flymake
> >> > would make that impossible, wouldn't it?
> >> 
> >> I don't think adding a command for fixing the diagnostic at point should
> >> preclude any other developments or explorations.  It's a useful thing to
> >> have, and many Flymake backends have the needed data readily available.
> >> 
> >> > So before any change in flymake I would like to see much more
> >> > exploration of "fix" UIs which are genuinely flymake-independent.
> >> 
> >> Flymake shows diagnostics, and "fixing" is what we do to diagnostics.
> >> What would be the benefit of a Flymake-independent UI for fixing the
> >> diagnostics that Flymake already shows?
> >
> > The benefit would be that we will be able to use that UI when "fixes"
> > are shown in, for example, the *compilation* buffer.
> 
> It'd be good to enhance compilation buffers as well, but this feature
> request is about interaction with Flymake diagnostics, that are shown in
> the diagnosed buffer: I'd like to have a standard way to act on (fix)
> the diagnostic at point.

I frankly don't understand what you are saying here.  Several people
opined that we should take a broader view on the fixes and how to
handle them, but you insist that Flymake should have its own solution?

IOW, the "fixes" diagnostic shown by Flymake is not just diagnostic,
it's a suggestion to make some change in the source code.  So
supporting that cannot be separated from the more general concept of
making changes proposed by some external tool.  Or what am I missing?

Or maybe this is a simple misunderstanding: what do you mean by
"acting on diagnostic at point", and how could such an act be
indifferent to what and how is fixed?





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-07 10:44           ` Eli Zaretskii
@ 2024-07-07 11:50             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-07 14:28               ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-07-07 11:50 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: sbaugh, 71504

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
>> Date: Sun, 07 Jul 2024 10:53:12 +0200
>> 
>> >> From: Eshel Yaron <me@eshelyaron.com>
>> >> Cc: Eli Zaretskii <eliz@gnu.org>,  71504@debbugs.gnu.org
>> >> Date: Thu, 27 Jun 2024 20:15:37 +0200
>> >> 
>> >> Spencer Baugh <sbaugh@janestreet.com> writes:
>> >>
>> >> > For example, maybe we want to have a command which can accept fixes
>> >> > output by a process running in M-x compile.  Baking the UI into flymake
>> >> > would make that impossible, wouldn't it?
>> >> 
>> >> I don't think adding a command for fixing the diagnostic at point should
>> >> preclude any other developments or explorations.  It's a useful thing to
>> >> have, and many Flymake backends have the needed data readily available.
>> >> 
>> >> > So before any change in flymake I would like to see much more
>> >> > exploration of "fix" UIs which are genuinely flymake-independent.
>> >> 
>> >> Flymake shows diagnostics, and "fixing" is what we do to diagnostics.
>> >> What would be the benefit of a Flymake-independent UI for fixing the
>> >> diagnostics that Flymake already shows?
>> >
>> > The benefit would be that we will be able to use that UI when "fixes"
>> > are shown in, for example, the *compilation* buffer.
>> 
>> It'd be good to enhance compilation buffers as well, but this feature
>> request is about interaction with Flymake diagnostics, that are shown in
>> the diagnosed buffer: I'd like to have a standard way to act on (fix)
>> the diagnostic at point.
>
> I frankly don't understand what you are saying here.  Several people
> opined that we should take a broader view on the fixes and how to
> handle them, but you insist that Flymake should have its own solution?

No.  I only insist that there should be a command for fixing the
Flymake diagnostic at point.  If it's part of a "broader solution",
that's swell.

> IOW, the "fixes" diagnostic shown by Flymake is not just diagnostic,
> it's a suggestion to make some change in the source code.

I think there is a misunderstanding here: it's not about specific
diagnostics which represent fixes, this is about enriching
(potentially) all diagnostics with backend-provided fix suggestions,
and adding a command that applies such fixes.  For example, with my
implementation I use the same command for fixing checkdoc, shellcheck
and LSP diagnostics.

> So supporting that cannot be separated from the more general concept
> of making changes proposed by some external tool.  Or what am I
> missing?

IIUC, I think I agree.  In my implementation, Flymake delegates the
application of the code changes to another library, that includes a
general purpose function for applying code changes.

> Or maybe this is a simple misunderstanding: what do you mean by
> "acting on diagnostic at point"

Applying a suggested code change that resolves the diagnostic.

> , and how could such an act be indifferent to what and how is fixed?

A single command should let you fix diagnostics from different sources
(backends).  It doesn't need to be indifferent, just consistent.

Does that make sense?





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-07 11:50             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-07-07 14:28               ` Eli Zaretskii
  2024-07-11  5:43                 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-07-07 14:28 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: sbaugh, 71504

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
> Date: Sun, 07 Jul 2024 13:50:35 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> It'd be good to enhance compilation buffers as well, but this feature
> >> request is about interaction with Flymake diagnostics, that are shown in
> >> the diagnosed buffer: I'd like to have a standard way to act on (fix)
> >> the diagnostic at point.
> >
> > I frankly don't understand what you are saying here.  Several people
> > opined that we should take a broader view on the fixes and how to
> > handle them, but you insist that Flymake should have its own solution?
> 
> No.  I only insist that there should be a command for fixing the
> Flymake diagnostic at point.  If it's part of a "broader solution",
> that's swell.
> 
> > IOW, the "fixes" diagnostic shown by Flymake is not just diagnostic,
> > it's a suggestion to make some change in the source code.
> 
> I think there is a misunderstanding here: it's not about specific
> diagnostics which represent fixes, this is about enriching
> (potentially) all diagnostics with backend-provided fix suggestions,
> and adding a command that applies such fixes.  For example, with my
> implementation I use the same command for fixing checkdoc, shellcheck
> and LSP diagnostics.
> 
> > So supporting that cannot be separated from the more general concept
> > of making changes proposed by some external tool.  Or what am I
> > missing?
> 
> IIUC, I think I agree.  In my implementation, Flymake delegates the
> application of the code changes to another library, that includes a
> general purpose function for applying code changes.
> 
> > Or maybe this is a simple misunderstanding: what do you mean by
> > "acting on diagnostic at point"
> 
> Applying a suggested code change that resolves the diagnostic.
> 
> > , and how could such an act be indifferent to what and how is fixed?
> 
> A single command should let you fix diagnostics from different sources
> (backends).  It doesn't need to be indifferent, just consistent.
> 
> Does that make sense?

It sounds like we all agree, but then what is the problem?





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-07 14:28               ` Eli Zaretskii
@ 2024-07-11  5:43                 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-11  6:08                   ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-07-11  5:43 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: sbaugh, 71504

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

> It sounds like we all agree, but then what is the problem?

I'm not sure I understand what you're asking, I don't think there's any
problem here per se, just a need for a decision on how to proceed:

If you and Spencer agree with my overall implementation, I'll polish it
a bit and post an updated patch for master.  If you want to implement
the requested feature in some other way, that's perfectly fine too.
Lastly, if you don't think Emacs should provide a command for applying
fixes to Flymake diagnostics, feel free to close this feature request.


Thanks,

Eshel





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-11  5:43                 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-07-11  6:08                   ` Eli Zaretskii
  2024-07-11  7:28                     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-07-11  6:08 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: sbaugh, 71504

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
> Date: Thu, 11 Jul 2024 07:43:53 +0200
> 
> Hi Eli,
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It sounds like we all agree, but then what is the problem?
> 
> I'm not sure I understand what you're asking, I don't think there's any
> problem here per se, just a need for a decision on how to proceed:
> 
> If you and Spencer agree with my overall implementation, I'll polish it
> a bit and post an updated patch for master.  If you want to implement
> the requested feature in some other way, that's perfectly fine too.
> Lastly, if you don't think Emacs should provide a command for applying
> fixes to Flymake diagnostics, feel free to close this feature request.

I'm asking what is the overall idea of the proposed implementation.  I
think it's worthwhile to present it, so we could see if we all agree
with that idea and the details of the proposed implementation.

The reason I think we should go back and discuss that is that in the
beginning there seemed to be some disagreement, but after several
messages it actually sounds like we all agree.  So it would be good to
go back and examine the original idea again.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-11  6:08                   ` Eli Zaretskii
@ 2024-07-11  7:28                     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-12  6:35                       ` Eli Zaretskii
  2024-07-16 21:27                       ` Spencer Baugh
  0 siblings, 2 replies; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-07-11  7:28 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: sbaugh, 71504

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
>> Date: Thu, 11 Jul 2024 07:43:53 +0200
>> 
>> Hi Eli,
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > It sounds like we all agree, but then what is the problem?
>> 
>> I'm not sure I understand what you're asking, I don't think there's any
>> problem here per se, just a need for a decision on how to proceed:
>> 
>> If you and Spencer agree with my overall implementation, I'll polish it
>> a bit and post an updated patch for master.  If you want to implement
>> the requested feature in some other way, that's perfectly fine too.
>> Lastly, if you don't think Emacs should provide a command for applying
>> fixes to Flymake diagnostics, feel free to close this feature request.
>
> I'm asking what is the overall idea of the proposed implementation.  I
> think it's worthwhile to present it, so we could see if we all agree
> with that idea and the details of the proposed implementation.

Thanks.  To clarify, ideally Spencer will implement this feature request
however he sees fit.  I'm offering my implementation as a reference, but
I'm not advocating for it over other alternatives that may come up.

The idea of my implementation is to allow Flymake backends to associate
fixes with some of the diagnostics they create, and to add a command
that tries to apply a fix for the diagnostic at point.  For the details,
see below the same patch I attached to this message:
https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01318.html

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 2e602658ea7..cf5349765db 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -368,7 +368,7 @@ flymake-error
   locus beg end type text backend data overlay-properties overlay
   ;; FIXME: See usage of these two in `flymake--highlight-line'.
   ;; Ideally they wouldn't be needed.
-  orig-beg orig-end)
+  orig-beg orig-end fix-function)
 
 ;;;###autoload
 (defun flymake-make-diagnostic (locus
@@ -377,7 +377,8 @@ flymake-make-diagnostic
                                 type
                                 text
                                 &optional data
-                                overlay-properties)
+                                overlay-properties
+                                fix-function)
   "Make a Flymake diagnostic for LOCUS's region from BEG to END.
 LOCUS is a buffer object or a string designating a file name.
 
@@ -396,14 +397,24 @@ flymake-make-diagnostic
 OVERLAY-PROPERTIES is an alist of properties attached to the
 created diagnostic, overriding the default properties and any
 properties listed in the `flymake-overlay-control' property of
-the diagnostic's type symbol."
+the diagnostic's type symbol.
+
+FIX-FUNCTION, if non-nil, is a function that takes DATA and returns a
+list of fix suggestions for this diagnostic.  Each fix suggestion is a
+list (TITLE EDITS), where TITLE is a string describing the fix and EDITS
+is a list of (FILE-OR-BUFFER . CHANGES) cons cells, where FILE-OR-BUFFER
+is the file name or buffer to edit, and CHANGES is a list of changes to
+perform in FILE-OR-BUFFER.  Each element of CHANGES is in turn a
+list (BEG END STR), where BEG and END are buffer positions to delete and
+STR is the string to insert at BEG afterwards."
   (when (stringp locus)
     (setq locus (expand-file-name locus)))
   (flymake--diag-make :locus locus :beg beg :end end
                       :type type :text text :data data
                       :overlay-properties overlay-properties
                       :orig-beg beg
-                      :orig-end end))
+                      :orig-end end
+                      :fix-function fix-function))
 
 ;;;###autoload
 (defun flymake-diagnostics (&optional beg end)
@@ -849,6 +860,42 @@ flymake--update-eol-overlays
             (overlay-put o 'before-string (flymake--eol-overlay-summary src-ovs))
           (delete-overlay o))))))
 
+(defun flymake-diagnostic-context-menu (menu click)
+  "Extend MENU with fix suggestions for diagnostic at CLICK."
+  (when-let ((diag (seq-find #'flymake--diag-fix-function
+                             (flymake-diagnostics
+                              (posn-point (event-start click)))))
+             (fixes (funcall (flymake--diag-fix-function diag)
+                             (flymake--diag-data diag)))
+             (i 1))
+    (dolist (fix fixes)
+      (define-key menu (vector (intern (format "flymake-fix-%d" i)))
+                  `(menu-item ,(format "Fix: %s" (car fix))
+                              ,(lambda ()
+                                 (interactive)
+                                 (refactor-apply-edits (cadr fix)))
+                              ,@(cddr fix)))
+      (cl-incf i)))
+  menu)
+
+(defun flymake-fix (pos)
+  "Fix Flymake diagnostic at POS."
+  (interactive "d")
+  ;; TODO - fix _all_ diagnostics at point.
+  (if-let ((diags (flymake-diagnostics pos)))
+      (if-let ((diag (seq-find #'flymake--diag-fix-function diags))
+               (fixes (funcall (flymake--diag-fix-function diag)
+                               (flymake--diag-data diag))))
+          (refactor-apply-edits
+           (car (if (cdr fixes)
+                    (alist-get
+                     (completing-read (format-prompt "Fix" (caar fixes))
+                                      fixes nil t nil nil (caar fixes))
+                     fixes nil nil #'string=)
+                  (cdar fixes))))
+        (message "No fix available for this diagnostic"))
+    (user-error "No diagnostic at this position")))
+
 (cl-defun flymake--highlight-line (diagnostic &optional foreign)
   "Attempt to overlay DIAGNOSTIC in current buffer.
 
@@ -956,6 +1003,7 @@ flymake--highlight-line
              (flymake-diagnostics pos)
              "\n"))))
       (default-maybe 'severity (warning-numeric-level :error))
+      (default-maybe 'context-menu-functions '(flymake-diagnostic-context-menu))
       ;; Use (PRIMARY . SECONDARY) priority, to avoid clashing with
       ;; `region' face, for example (bug#34022).
       (default-maybe 'priority (cons nil (+ 40 (overlay-get ov 'severity)))))
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index a348e9ba6fd..fccee14af82 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -3195,6 +3195,21 @@ sh-shellcheck-arguments
 
 (defvar-local sh--shellcheck-process nil)
 
+(defun sh-shellcheck-fix (data)
+  "Format DATA, a cons cell (TITLE . FIX), as a Flymake fix suggestion."
+  `((,(car data)
+     ((,(current-buffer)
+       ,@(mapcar
+          (lambda (rep)
+            (let-alist rep
+              (without-restriction
+                (save-excursion
+                  (goto-char (point-min))
+                  (list (1- (+ (pos-bol .line) .column))
+                        (1- (+ (pos-bol .endLine) .endColumn))
+                        .replacement)))))
+          (alist-get 'replacements (cdr data))))))))
+
 (defun sh-shellcheck-flymake (report-fn &rest _args)
   "Flymake backend using the shellcheck program.
 Takes a Flymake callback REPORT-FN as argument, as expected of a
@@ -3236,7 +3251,9 @@ sh-shellcheck-flymake
                                 ("error" :error)
                                 ("warning" :warning)
                                 (_ :note))
-                              (format "SC%s: %s" .code .message)))))
+                              (format "SC%s: %s" .code .message)
+                              (cons .message .fix) nil
+                              (when (consp .fix) #'sh-shellcheck-fix)))))
                         (funcall report-fn))))
                 (kill-buffer (process-buffer proc)))))))
     (unless dialect





^ permalink raw reply related	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-11  7:28                     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-07-12  6:35                       ` Eli Zaretskii
  2024-07-16  9:49                         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-16 21:27                       ` Spencer Baugh
  1 sibling, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-07-12  6:35 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: sbaugh, 71504

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
> Date: Thu, 11 Jul 2024 09:28:35 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm asking what is the overall idea of the proposed implementation.  I
> > think it's worthwhile to present it, so we could see if we all agree
> > with that idea and the details of the proposed implementation.
> 
> Thanks.  To clarify, ideally Spencer will implement this feature request
> however he sees fit.  I'm offering my implementation as a reference, but
> I'm not advocating for it over other alternatives that may come up.
> 
> The idea of my implementation is to allow Flymake backends to associate
> fixes with some of the diagnostics they create, and to add a command
> that tries to apply a fix for the diagnostic at point.  For the details,
> see below the same patch I attached to this message:
> https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01318.html

Thanks.

If this is okay with Spencer, I think this should go to the master
branch, with the following two nits fixed:

  . the doc string of flymake-make-diagnostic should explicitly tell
    that :fix-function is for backends to be set to the appropriate
    fixup function
  . this is documented in flymake.texi





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-12  6:35                       ` Eli Zaretskii
@ 2024-07-16  9:49                         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-16 10:28                           ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-07-16  9:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: sbaugh, 71504

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
>> Date: Thu, 11 Jul 2024 09:28:35 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I'm asking what is the overall idea of the proposed implementation.  I
>> > think it's worthwhile to present it, so we could see if we all agree
>> > with that idea and the details of the proposed implementation.
>> 
>> Thanks.  To clarify, ideally Spencer will implement this feature request
>> however he sees fit.  I'm offering my implementation as a reference, but
>> I'm not advocating for it over other alternatives that may come up.
>> 
>> The idea of my implementation is to allow Flymake backends to associate
>> fixes with some of the diagnostics they create, and to add a command
>> that tries to apply a fix for the diagnostic at point.  For the details,
>> see below the same patch I attached to this message:
>> https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01318.html
>
> Thanks.
>
> If this is okay with Spencer, I think this should go to the master
> branch, with the following two nits fixed:
>
>   . the doc string of flymake-make-diagnostic should explicitly tell
>     that :fix-function is for backends to be set to the appropriate
>     fixup function
>   . this is documented in flymake.texi

Thanks, will do.  Note that to apply a fix suggestion, this patch uses
function refactor-apply-edits from my library refactor.el.  This is the
"general purpose function for applying code changes" I mentioned in a
previous message in this thread.  So to land this on master we need to
also add (at least a part of) refactor.el.  I'm happy to contribute the
library wholesale, FWIW.

You can find the latest version of refactor.el here:

https://git.sr.ht/~eshel/emacs/blob/main/lisp/progmodes/refactor.el


Regards,

Eshel





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-16  9:49                         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-07-16 10:28                           ` Eli Zaretskii
  2024-07-16 15:19                             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-07-16 10:28 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: sbaugh, 71504

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
> Date: Tue, 16 Jul 2024 11:49:45 +0200
> 
> > If this is okay with Spencer, I think this should go to the master
> > branch, with the following two nits fixed:
> >
> >   . the doc string of flymake-make-diagnostic should explicitly tell
> >     that :fix-function is for backends to be set to the appropriate
> >     fixup function
> >   . this is documented in flymake.texi
> 
> Thanks, will do.  Note that to apply a fix suggestion, this patch uses
> function refactor-apply-edits from my library refactor.el.  This is the
> "general purpose function for applying code changes" I mentioned in a
> previous message in this thread.  So to land this on master we need to
> also add (at least a part of) refactor.el.  I'm happy to contribute the
> library wholesale, FWIW.
> 
> You can find the latest version of refactor.el here:
> 
> https://git.sr.ht/~eshel/emacs/blob/main/lisp/progmodes/refactor.el

Couldn't refactor-apply-edits be replaced with something that doesn't
depend on the rest of the library?  I'd prefer not to delay the fixit
support in Eglot until we decide whether or not to add refactor.el.
Adding refactor.el could then be a separate discussion (and I do think
having refactoring capabilities in Emacs is long overdue).

Thanks.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-16 10:28                           ` Eli Zaretskii
@ 2024-07-16 15:19                             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-16 15:33                               ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-07-16 15:19 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: sbaugh, 71504

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
>> Date: Tue, 16 Jul 2024 11:49:45 +0200
>> 
>> > If this is okay with Spencer, I think this should go to the master
>> > branch, with the following two nits fixed:
>> >
>> >   . the doc string of flymake-make-diagnostic should explicitly tell
>> >     that :fix-function is for backends to be set to the appropriate
>> >     fixup function
>> >   . this is documented in flymake.texi
>> 
>> Thanks, will do.  Note that to apply a fix suggestion, this patch uses
>> function refactor-apply-edits from my library refactor.el.  This is the
>> "general purpose function for applying code changes" I mentioned in a
>> previous message in this thread.  So to land this on master we need to
>> also add (at least a part of) refactor.el.  I'm happy to contribute the
>> library wholesale, FWIW.
>> 
>> You can find the latest version of refactor.el here:
>> 
>> https://git.sr.ht/~eshel/emacs/blob/main/lisp/progmodes/refactor.el
>
> Couldn't refactor-apply-edits be replaced with something that doesn't
> depend on the rest of the library?

Yes, that's certainly doable.  refactor-apply-edits can apply edits in
different ways depending on the value of refactor-apply-edits-function:

1. Apply all edits at once, without further confirmation.
2. Preview changes inline, and query about applying each change in turn.
   (This is like query-replace with inline before/after preview.)
3. Summarize edits in a diff buffer, let the user take it from there.

I guess that for applying fix suggestions for Flymake diagnostics we can
extract just the couple of functions that apply all edits at once.

> I'd prefer not to delay the fixit support in Eglot until we decide
> whether or not to add refactor.el.  Adding refactor.el could then be a
> separate discussion (and I do think having refactoring capabilities in
> Emacs is long overdue).

SGTM.  (I assume you meant Flymake rather than Eglot?)

I'll put together an updated patch once Spencer confirms.


Eshel






^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-16 15:19                             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-07-16 15:33                               ` Eli Zaretskii
  0 siblings, 0 replies; 22+ messages in thread
From: Eli Zaretskii @ 2024-07-16 15:33 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: sbaugh, 71504

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
> Date: Tue, 16 Jul 2024 17:19:47 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'd prefer not to delay the fixit support in Eglot until we decide
> > whether or not to add refactor.el.  Adding refactor.el could then be a
> > separate discussion (and I do think having refactoring capabilities in
> > Emacs is long overdue).
> 
> SGTM.  (I assume you meant Flymake rather than Eglot?)

Yes, sorry.

> I'll put together an updated patch once Spencer confirms.

Thanks.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-11  7:28                     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-12  6:35                       ` Eli Zaretskii
@ 2024-07-16 21:27                       ` Spencer Baugh
  2024-07-17 11:51                         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 22+ messages in thread
From: Spencer Baugh @ 2024-07-16 21:27 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: Eli Zaretskii, 71504

Eshel Yaron <me@eshelyaron.com> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Eshel Yaron <me@eshelyaron.com>
>>> Cc: sbaugh@janestreet.com,  71504@debbugs.gnu.org
>>> Date: Thu, 11 Jul 2024 07:43:53 +0200
>>> 
>>> Hi Eli,
>>> 
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>> 
>>> > It sounds like we all agree, but then what is the problem?
>>> 
>>> I'm not sure I understand what you're asking, I don't think there's any
>>> problem here per se, just a need for a decision on how to proceed:
>>> 
>>> If you and Spencer agree with my overall implementation, I'll polish it
>>> a bit and post an updated patch for master.  If you want to implement
>>> the requested feature in some other way, that's perfectly fine too.
>>> Lastly, if you don't think Emacs should provide a command for applying
>>> fixes to Flymake diagnostics, feel free to close this feature request.
>>
>> I'm asking what is the overall idea of the proposed implementation.  I
>> think it's worthwhile to present it, so we could see if we all agree
>> with that idea and the details of the proposed implementation.
>
> Thanks.  To clarify, ideally Spencer will implement this feature request
> however he sees fit.  I'm offering my implementation as a reference, but
> I'm not advocating for it over other alternatives that may come up.
>
> The idea of my implementation is to allow Flymake backends to associate
> fixes with some of the diagnostics they create, and to add a command
> that tries to apply a fix for the diagnostic at point.  For the details,
> see below the same patch I attached to this message:
> https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01318.html

A few issues:

- At an immediate glance, fix-function should return a cl-defstruct
instead of a list, to match the rest of the flymake API.

- An implementation of this feature in flymake absolutely must come with
support for Eglot, as the main user of this feature.  Which, if the
Eglot maintainer doesn't want to merge that, may mean we can't move
forward.

- Your patch adds support in shellcheck for fixes.  That's
uncontroversially useful and good.  Could we add this support in a
shellcheck-specific way before finalizing the flymake fix API?  (Taking
care to add it in a way that can be supported by a later unified UI, of
course)

- Likewise, you mentioned adding support for fixes to checkdoc, although
I'm not sure where that patch is.  That sounds also extremely useful,
could it also be added first?

- Do you hope to have a default binding for the fix-applying command you
mention?  Certainly I would like that, and I think it's worth talking
about now.

More broadly, I'm still a bit unsure about this whole approach.

- What UI, exactly, do you want to build on top of this?  Can we see an
example of this UI?  Or is it really just this one command?

- If it's just the one command, and your hope is to give this some
default binding, what exactly is the problem with doing that via a
keymap overlay as Joao suggests?  What do you want to do which can't be
done with a keymap overlay bound to a backend-specific function?

- Could this UI also support spell-checking, via ispell or flyspell?  It
seems like "fix the spelling of a typo'd word" is something that would
very naturally fit this whole idea.

- Could we implement all this outside of flymake, I wonder, with some
kind of refactor-backend-functions buffer-local?





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-16 21:27                       ` Spencer Baugh
@ 2024-07-17 11:51                         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-07-24 16:40                           ` Spencer Baugh
  0 siblings, 1 reply; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-07-17 11:51 UTC (permalink / raw)
  To: Spencer Baugh; +Cc: Eli Zaretskii, 71504

Hi Spencer,

Spencer Baugh <sbaugh@janestreet.com> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>> I'm asking what is the overall idea of the proposed implementation.  I
>>> think it's worthwhile to present it, so we could see if we all agree
>>> with that idea and the details of the proposed implementation.
>>
>> Thanks.  To clarify, ideally Spencer will implement this feature request
>> however he sees fit.  I'm offering my implementation as a reference, but
>> I'm not advocating for it over other alternatives that may come up.
>>
>> The idea of my implementation is to allow Flymake backends to associate
>> fixes with some of the diagnostics they create, and to add a command
>> that tries to apply a fix for the diagnostic at point.  For the details,
>> see below the same patch I attached to this message:
>> https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01318.html
>
> A few issues:
>
> - At an immediate glance, fix-function should return a cl-defstruct
> instead of a list, to match the rest of the flymake API.

The fix-function returns any number of fixes, as a list of fixes.  I
guess that's not the list you're referring to, because I don't see how
this list (whose number of elements vary) can be replaced by a
cl-defstruct.  Each individual fix is also represented as a list, with
exactly two elements: TITLE and EDITS.  Is this what you'd like to see
replaced with a cl-defstruct?

> - An implementation of this feature in flymake absolutely must come with
> support for Eglot, as the main user of this feature.  Which, if the
> Eglot maintainer doesn't want to merge that, may mean we can't move
> forward.

I agree that Eglot support is important (it was naturally the first
backend I adapted), and that it'll be great to have João on board.
I don't think it's a blocker, but it's your call.

> - Your patch adds support in shellcheck for fixes.  That's
> uncontroversially useful and good.  Could we add this support in a
> shellcheck-specific way before finalizing the flymake fix API?  (Taking
> care to add it in a way that can be supported by a later unified UI, of
> course)

Maybe, but it would be less useful: it wouldn't help other backends,
including third-party backends, to provide fixes.  Also, I don't think
there's anything final here, there's plenty of room for developing the
API further if more/different requirements arise.

> - Likewise, you mentioned adding support for fixes to checkdoc, although
> I'm not sure where that patch is.

It's on my development branch, namely this commit:

http://git.eshelyaron.com/gitweb/?p=emacs.git;a=commitdiff;h=650c2a056af8df85065b2851d3513c1e3d62c60c

> That sounds also extremely useful, could it also be added first?

First as in not via Flymake?  Note that checkdoc already has its own fix
support, the point here is to add something that'll work consistently
across backends.

> - Do you hope to have a default binding for the fix-applying command you
> mention?  Certainly I would like that, and I think it's worth talking
> about now.

IMO it's fine to leave it unbound at first, and see how users bind it.
But if you find an appropriate available binding, why not? :)

> More broadly, I'm still a bit unsure about this whole approach.
>
> - What UI, exactly, do you want to build on top of this?  Can we see an
> example of this UI?  Or is it really just this one command?

A simple command is a good start.  Once diagnostics are enriched with
fix suggestions, folks can add more bells and whistles if they like to.

FWIW, two further small enhancements that I'm experimenting with are:
1. Apply fixes from the diagnostics list buffer.
2. Apply fixes to one or more diagnostics that you choose with completion.

> - If it's just the one command, and your hope is to give this some
> default binding, what exactly is the problem with doing that via a
> keymap overlay as Joao suggests?  What do you want to do which can't be
> done with a keymap overlay bound to a backend-specific function?

As we've already established, individual backends can do anything at all
via overlay properties and other methods.  But that puts extraneous
burden on backends to deal with frontend concerns, and it doesn't yield
a consistent UI across backends (one command that works everywhere).

> - Could this UI also support spell-checking, via ispell or flyspell?  It
> seems like "fix the spelling of a typo'd word" is something that would
> very naturally fit this whole idea.

Sure.

> - Could we implement all this outside of flymake, I wonder, with some
> kind of refactor-backend-functions buffer-local?

Dunno, probably, somehow.  But there's no refactor.el in core, and,
however we do it, it'll need to interact with Flymake diagnostics that
come from Flymake backends, so flymake.el seems like a natural choice.
Crucially, in flymake.el we can extend flymake-make-diagnostic to make
it as easy as possible for backends to provide fixes.


Eshel





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-17 11:51                         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-07-24 16:40                           ` Spencer Baugh
  2024-07-24 17:44                             ` João Távora
  2024-07-25  9:04                             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 2 replies; 22+ messages in thread
From: Spencer Baugh @ 2024-07-24 16:40 UTC (permalink / raw)
  To: Eshel Yaron; +Cc: Eli Zaretskii, 71504, joaotavora

Eshel Yaron <me@eshelyaron.com> writes:

> Hi Spencer,
>
> Spencer Baugh <sbaugh@janestreet.com> writes:
>
>> Eshel Yaron <me@eshelyaron.com> writes:
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>
>>>> I'm asking what is the overall idea of the proposed implementation.  I
>>>> think it's worthwhile to present it, so we could see if we all agree
>>>> with that idea and the details of the proposed implementation.
>>>
>>> Thanks.  To clarify, ideally Spencer will implement this feature request
>>> however he sees fit.  I'm offering my implementation as a reference, but
>>> I'm not advocating for it over other alternatives that may come up.
>>>
>>> The idea of my implementation is to allow Flymake backends to associate
>>> fixes with some of the diagnostics they create, and to add a command
>>> that tries to apply a fix for the diagnostic at point.  For the details,
>>> see below the same patch I attached to this message:
>>> https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01318.html
>>
>> A few issues:
>>
>> - At an immediate glance, fix-function should return a cl-defstruct
>> instead of a list, to match the rest of the flymake API.
>
> The fix-function returns any number of fixes, as a list of fixes.  I
> guess that's not the list you're referring to, because I don't see how
> this list (whose number of elements vary) can be replaced by a
> cl-defstruct.  Each individual fix is also represented as a list, with
> exactly two elements: TITLE and EDITS.  Is this what you'd like to see
> replaced with a cl-defstruct?

Yes.

>> - An implementation of this feature in flymake absolutely must come with
>> support for Eglot, as the main user of this feature.  Which, if the
>> Eglot maintainer doesn't want to merge that, may mean we can't move
>> forward.
>
> I agree that Eglot support is important (it was naturally the first
> backend I adapted), and that it'll be great to have João on board.
> I don't think it's a blocker, but it's your call.

I don't want to be obstructionist, but it seems to me that if we land
this in flymake, eventually it will just *have* to be supported in
Eglot.  I don't want to abuse that as a way to work around what Joao
prefers.

>> - Your patch adds support in shellcheck for fixes.  That's
>> uncontroversially useful and good.  Could we add this support in a
>> shellcheck-specific way before finalizing the flymake fix API?  (Taking
>> care to add it in a way that can be supported by a later unified UI, of
>> course)
>
> Maybe, but it would be less useful: it wouldn't help other backends,
> including third-party backends, to provide fixes.  Also, I don't think
> there's anything final here, there's plenty of room for developing the
> API further if more/different requirements arise.

Of course that's all true, but also it doesn't seem harmful to just add
some shellcheck-apply-fix command, which contains all the
shellcheck-specific logic you already need to add.  It will have a tiny
bit of duplication, but we can get rid of that after adding a unified
UI.

>> - Likewise, you mentioned adding support for fixes to checkdoc, although
>> I'm not sure where that patch is.
>
> It's on my development branch, namely this commit:
>
> http://git.eshelyaron.com/gitweb/?p=emacs.git;a=commitdiff;h=650c2a056af8df85065b2851d3513c1e3d62c60c

Is this server down right now?

>> That sounds also extremely useful, could it also be added first?
>
> First as in not via Flymake?  Note that checkdoc already has its own fix
> support, the point here is to add something that'll work consistently
> across backends.

Ah, I'll have to judge when I see the commit.

>> - Do you hope to have a default binding for the fix-applying command you
>> mention?  Certainly I would like that, and I think it's worth talking
>> about now.
>
> IMO it's fine to leave it unbound at first, and see how users bind it.
> But if you find an appropriate available binding, why not? :)
>
>> More broadly, I'm still a bit unsure about this whole approach.
>>
>> - What UI, exactly, do you want to build on top of this?  Can we see an
>> example of this UI?  Or is it really just this one command?
>
> A simple command is a good start.  Once diagnostics are enriched with
> fix suggestions, folks can add more bells and whistles if they like to.
>
> FWIW, two further small enhancements that I'm experimenting with are:
> 1. Apply fixes from the diagnostics list buffer.
> 2. Apply fixes to one or more diagnostics that you choose with completion.
>
>> - If it's just the one command, and your hope is to give this some
>> default binding, what exactly is the problem with doing that via a
>> keymap overlay as Joao suggests?  What do you want to do which can't be
>> done with a keymap overlay bound to a backend-specific function?
>
> As we've already established, individual backends can do anything at all
> via overlay properties and other methods.  But that puts extraneous
> burden on backends to deal with frontend concerns, and it doesn't yield
> a consistent UI across backends (one command that works everywhere).
>
>> - Could this UI also support spell-checking, via ispell or flyspell?  It
>> seems like "fix the spelling of a typo'd word" is something that would
>> very naturally fit this whole idea.
>
> Sure.

My implicit point here is that ispell/flyspell aren't flymake-based, so
something tied to flymake may make this harder.  (But maybe
flymake-based spellcheckers are better anyway?  I don't actually use
flyspell or any other on-the-fly spellchecker, so I can't really say)

>> - Could we implement all this outside of flymake, I wonder, with some
>> kind of refactor-backend-functions buffer-local?
>
> Dunno, probably, somehow.  But there's no refactor.el in core, and,
> however we do it, it'll need to interact with Flymake diagnostics that
> come from Flymake backends, so flymake.el seems like a natural choice.
> Crucially, in flymake.el we can extend flymake-make-diagnostic to make
> it as easy as possible for backends to provide fixes.

Well, sure, flymake is important.  But if we did it with some kind of
buffer-local list of backend functions, it could also have backends
which aren't flymake.  IDK if there are such backends though.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-24 16:40                           ` Spencer Baugh
@ 2024-07-24 17:44                             ` João Távora
  2024-07-25  9:04                             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 0 replies; 22+ messages in thread
From: João Távora @ 2024-07-24 17:44 UTC (permalink / raw)
  To: Spencer Baugh; +Cc: Eli Zaretskii, Eshel Yaron, 71504

On Wed, Jul 24, 2024 at 5:40 PM Spencer Baugh <sbaugh@janestreet.com> wrote:

> I don't want to be obstructionist, but it seems to me that if we land
> this in flymake, eventually it will just *have* to be supported in
> Eglot.

Not aware that Eglot is under such obligation.  It generally wants
to use Emacs facilities that do useful work, but only if those things
are simpler than existing battle-tested things.  This is not one of
those.

The patch Eshel was proposing adds a re-representation of
code actions to Flymake, something completely outside its
responsibilities (as I designed and maintained for a number of
years at least).

Flymake's current API is fine for Eglot's code-action purposes.
A refactor.el API would be even better.

> I don't want to abuse that as a way to work around what Joao
> prefers.

I suggested an alternative idea to put in the refactor.el library.  No idea
why that's not being considered : ¯\_(ツ)_/¯

https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01163.html

Then for sure Eglot would migrate, as I'm always looking for less
code to maintain, not more.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics
  2024-07-24 16:40                           ` Spencer Baugh
  2024-07-24 17:44                             ` João Távora
@ 2024-07-25  9:04                             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 0 replies; 22+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-07-25  9:04 UTC (permalink / raw)
  To: Spencer Baugh; +Cc: Eli Zaretskii, 71504, joaotavora

Hi Spencer,

Spencer Baugh <sbaugh@janestreet.com> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>
>>> - Likewise, you mentioned adding support for fixes to checkdoc, although
>>> I'm not sure where that patch is.
>>
>> It's on my development branch, namely this commit:
>>
>> http://git.eshelyaron.com/gitweb/?p=emacs.git;a=commitdiff;h=650c2a056af8df85065b2851d3513c1e3d62c60c
>
> Is this server down right now?

Doesn't seem so, 180 days of uptime :)
There's also a mirror here if you have troubles reaching my server:

https://git.sr.ht/~eshel/emacs/commit/650c2a056af8df85065b2851d3513c1e3d62c60c





^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2024-07-25  9:04 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-12  8:43 bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-06-27  7:32 ` Eli Zaretskii
2024-06-27 13:35   ` Spencer Baugh
2024-06-27 18:15     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-07-06  7:50       ` Eli Zaretskii
2024-07-07  8:53         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-07-07 10:44           ` Eli Zaretskii
2024-07-07 11:50             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-07-07 14:28               ` Eli Zaretskii
2024-07-11  5:43                 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-07-11  6:08                   ` Eli Zaretskii
2024-07-11  7:28                     ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-07-12  6:35                       ` Eli Zaretskii
2024-07-16  9:49                         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-07-16 10:28                           ` Eli Zaretskii
2024-07-16 15:19                             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-07-16 15:33                               ` Eli Zaretskii
2024-07-16 21:27                       ` Spencer Baugh
2024-07-17 11:51                         ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-07-24 16:40                           ` Spencer Baugh
2024-07-24 17:44                             ` João Távora
2024-07-25  9:04                             ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors

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