feat: to-pipe command #318

NJichev · 2023-11-01T16:36:17Z

Closes #55

Aims to add to and from pipe to next-ls

mhanberg · 2023-11-01T18:01:19Z

lib/next_ls/commands/from_pipe.ex

+  alias GenLSP.Structures.WorkspaceEdit
+
+  defp opts do
+    Schematic.map(%{


Please let me know if you have any feedback for the Schematic library! It can get the job done but it was first built with codegen in mind, so the ergonomics are a little off I think. Curious to hear your thought!

lib/next_ls/commands/from_pipe.ex

test/next_ls/commands/from_pipe_test.exs

mhanberg · 2023-11-01T18:07:48Z

test/next_ls/commands/to_pipe_test.exs

+    @test_scenarios [
+      {"to_list(Map.new)", "Map.new() |> to_list()"},
+      {"to_list(a, b, c)", "a |> to_list(b, c)"},
+      {"Foo.Bar.baz(foo, bar)", "foo |> Foo.Bar.baz(bar)"},
+      {"Foo.Bar.baz(foo, bar, Map.new())", "foo |> Foo.Bar.baz(bar, Map.new())"}
+    ]


love this 🙌

test/next_ls/commands/to_pipe_test.exs

lib/next_ls/commands/from_pipe.ex

mhanberg · 2023-11-01T18:13:18Z

Looking great at first glance! I'll wait til you mark as ready for review til i do a full... um.. review 😆

NJichev · 2023-11-01T18:16:16Z

lib/next_ls/commands/from_pipe.ex

+  end
+
+  def new(opts) do
+    {:ok, %{text: text, uri: uri, position: position}} = Schematic.unify(opts(), Map.new(opts))


Is it okay crashing the server if we get bad arguments, I should probably take care of getting an error from schematic as well.

I was going to suggest wrapping the whole handle_event callback in the NextLS module in a try/rescue, so that we can log an error to the user and not crash the whole server.

You can publish a window/showMessage notification (I think there are examples in the codebase), so that it actually presents an UI notification to the user's editor

mhanberg · 2023-11-01T20:04:57Z

lib/next_ls.ex

+      GenLSP.notify(lsp, %GenLSP.Notifications.WindowShowMessage{
+        params: %GenLSP.Structures.ShowMessageParams{
+          type: GenLSP.Enumerations.MessageType.warning(),
+          message: "[Next LS] #{command} command has crashed"


I would also log the exception, and the show message can be [Next LS] #{command} has failed, see the logs for more details

mhanberg · 2023-11-01T21:19:48Z

Can you also write a couple of full integration tests?

mhanberg · 2023-11-02T01:02:16Z

Okay, I tested this out and wasn't able to get it to work (after making some changes, I'll push them up on another branch so you can see, I rebased so i don't want to force push to your branch).

I think the integration tests will reveal where it breaks. I'll paste some of the errors I was seeing

to-pipe errors

NextLS: -32700: {[line: 1, column: 42], "missing terminator: } (for \"{\" starting at line 1)", ""}

from-pipe errors

NextLS: -32700: {[line: 1, column: 5], "syntax error before: ", "'|>'"}

mhanberg · 2023-11-02T01:04:10Z

Changes i made: fb33786

mhanberg · 2023-11-03T17:11:55Z

We can punt it to another PR, but I didn't realize that we can return this command from the code actions request from the client, so it shows up in the little light bulb menu

NJichev · 2023-11-10T15:33:18Z

Changes i made: fb33786

I have added the changes from this branch as well

NJichev · 2023-11-10T16:00:35Z

test/support/utils.ex

@@ -114,6 +114,27 @@ defmodule NextLS.Support.Utils do
    end
  end

+  defmacro assert_opened(


Not the best name, but I need to notify the server so that it loads the text for a certain URI into the documents assigns. Other things that I could do is to load the files preemptively with the initialization step. Another way would be to just expect the client to send the text as well? I think this is the cleanest though, just a little overhead in the tests.

seems fine, can you rename to did_open ?

and can you actually submit this as a separate PR, and add it to all the tests? then i can merge that and i won't have to approve your CI test runs every time you push

mhanberg · 2023-12-20T19:03:41Z

i'll try and review/demo this before the weekend

mhanberg · 2024-01-18T03:36:31Z

Tested this out, seems to break in quite a few cases.

I was able to turn parser = code |> new(opts) |> next_token() |> next_token() into parser = next_token(next_token(new(code, opts))), but not the other way around. No error, just did nothing.

trying to do it on with the cursor on parse_program or parser here results in an error

    case parse_program(parser) do
      {ast, %{errors: []}} ->
        {:ok, ast}

      {ast, %{errors: errors}} ->
        {:error, ast, errors}
    end
  end

The error being NextLS: -32700: {[line: 1, column: 34], "missing terminator: end (for \"do\" starting at line 1)", ""}

NJichev · 2024-01-18T12:18:24Z

Thanks for testing it out, I'll add those as test scenarios.

The first scenario no idea why it didn't work, I'll have to take a look, also I'll do a better job of reporting the errors.
The second scenario I think it's an easy fix - I just have to add do/end as tokens to the bracket counting as well and I think it'll work fine, since currently it doesn't know to get more pieces of code until it compiles the case.
I will think of similar cases as this one

mhanberg · 2024-02-16T22:50:44Z

You can use the error tolerant parser be been building (Spitfire) in Next LS now, so it might make this easier.

mhanberg · 2024-02-24T22:40:50Z

CleanShot.2024-02-24.at.17.39.17.mp4

found a case where it leaves a stray variable above the function it un-pipes

mhanberg · 2024-02-24T22:42:48Z

Also that one was a little odd in that it un-piped the begging of the pipe chain instead of the one under the cursor.

I would have expected this more or less

Enum.map(
  file |> NextLS.ASTHelpers.Variables.list_variable_references({line, col}),
  fn {_name, {startl..endl, startc..endc}} ->
    [file, startl, endl, startc, endc]
end)

I'm not sure if that is enough to stop this from merging, can improve it later

mhanberg · 2024-02-24T22:44:24Z

also putting the cursor here results in the error

#    👇
Enum.map(
  NextLS.ASTHelpers.Variables.list_variable_references(file, {line, col}),
  fn {_name, {startl..endl, startc..endc}} ->
    [file, startl, endl, startc, endc]
  end
)

[Error] [NextLS] ** (ArithmeticError) bad argument in arithmetic expression

mhanberg · 2024-02-24T22:47:12Z

I'll see if i can't get some time to address those test cases tonight

mhanberg · 2024-02-25T05:20:29Z

I have some local changes but haven't finished them yet, I'll try to get that done tomorrow.

mhanberg · 2024-02-26T00:20:10Z

Fixing a test and the nix flake and then it'll be mergeable.

I decided to include the Sourceror library as it has a lot of functions around positions that I was about to remake. As well as its zipper implementation which is useful

mhanberg · 2024-02-26T00:20:38Z

Also I broke out from-pipe into a second PR

Co-authored-by: Mitchell Hanberg <[email protected]>