feat: add remove debugger code action

[NJichev]

A Code Action that removes the following warning checks from Credo:

• Credo.Check.Warning.Dbg
• Credo.Check.Warning.IExPry
• Credo.Check.Warning.IoInspect
• Credo.Check.Warning.IoPuts
• Credo.Check.Warning.MixEnv

[NJichev]

Note this is a quick and dirty version, this would delete the whole line in the following scenarios:

IO.inspect(arg); foo

list |> Enum.map(...) |> IO.inspect() |> Enum.join()

I think it's fine as a starting point but I could rework it to get the start and end ranges of the expression from spitfire in this PR.

[mhanberg]

Couple things to think about.

I believe we want the text edit to be as precise as possible, meaning only reflects the code we are changing.

This is due to the code possibly having errors and when we reformat it, it will introduce error artifacts.

So I think we want to only format the node we are affecting, and create a text edit with that range.

Also an edge case, having two debugger statements on the same line, but you only want to remove one of the. Consider something like foo(dbg(arg1), IO.inspect(arg2))

[NJichev]

The problem with affecting only the debugger node is that it will break in the semicolon and the piping cases, i.e.: IO.inspect(arg, label: "foo"); expr You'll be left with ; expr instead of expr - with a bigger scope the formatter deals with this. Also you'll be stuck most likely with empty lines of indentation as well that way(which could be okay with the client).

[mhanberg]

I also realized that we should only remove the function call but not the argument.

So in this case removing the semicolon is not a problem, because the argument will still be there.

[mhanberg]

At least for pass through functions like IO.inspect and dbg

[mhanberg]

I think you might be able to assume the expression is terminated with a semicolon if it has the newlines key in the metadata is equal to 0 (meaning, not >=1 or not present at all)

[NJichev]

I also realized that we should only remove the function call but not the argument.

So in this case removing the semicolon is not a problem, because the argument will still be there.

Hmm but that would break cases such as this one:

def f(bar) do
  IO.inspect(bar)
  other_f(bar) |> IO.inspect()
  ... code
 end

In this use case applying the code action for the first inspect and removing the argument would be the correct thing to do imo. And if you don't want to remove the argument you can just use the pipe variant.

[mhanberg]

How does that break? I think I'm missing it haha

[NJichev]

I mean not break it 😅 But leave the variable there hanging.

[mhanberg]

I mean not break it 😅 But leave the variable there hanging.

I see, I think think this is preferable, as if you have them inside a function call, it would just straight remove the argument and change the semantics of the code.i frequently do this myself.

I think you also mentioned that you should just use the pipe operator to avoid this, but with regard to dbg, this actually changes the sematnics of the code if you do that, so i frequently wrap it on purpose, and that's where i'd like to have the code action, but its tedious to remove the wrapping call to dbg

[mhanberg]

https://github.com/elixir-tools/next-ls/assets/5523984/d3395dc5-b843-4f59-a1c2-faf9fff9677b

found an edge case

bundle_base |> dbg() |> NextLS.Runtime.BundledElixir.install(IO.inspect(logger), mix_home: mixhome) |> IO.inspect()

with this original code, 3 code actions appear, but they all seem to be for the piped IO.inspect call at the end. once you activate one of those, they turn into unique code actions.

when activated, they all worked, tho

[mhanberg]

That might be a credo issue tho, i see that the diagnostics for the two IO.inspects have the same range

[mhanberg]

I think we can probably get this in for the 0.21 release, but i hope to do that before the end of the week. no pressure tho if we can't get it over the line.

[mhanberg]

can you rebase and then i'll try this out?

[mhanberg]

pushed a change to use credo from rrrene/credo rather than my fork. i think i deleted that branch when he merged it

also pushed a change that adds an "empty fallback" to the min_by function. it would raise if you triggered the code actions in the editor again after fixing one of them, because it would still have the diagnostic that was just fixed.

i think in a followup PR we can make code actions that fix a diagnostic, remove the diagnostic as well, as its presumably fixed

[mhanberg]

this code action is going to drastically speed up my workflow (i'm a heavy puts debugger), thank you so much for grinding this one out!

another follow up, maybe include a code action for "remove all debugger statements"