lexical-lsp / lexical

Lexical is a next-generation elixir language server
776 stars 77 forks source link

Complete callables without parens if present in locals_without_parens #739

Closed zachallaun closed 1 month ago

zachallaun commented 1 month ago

This is scratching an itch I had while I was doing some work on another library.

Fixes #601

scohen commented 1 month ago

This fixes #601 right? Awesome!

zachallaun commented 1 month ago

@scohen Thanks for the feedback! Agreed on tests; it does need more.

I'm actually not 100% sure that assert is going to complete without parens because the ExUnit assertions aren't actually in locals_without_parens. That makes me wonder what the formatter rules are for macros in built-in packages. Do they all exclude parens?

scohen commented 1 month ago

I tried this branch, and it still adds parens to all the exunit macros. I think this is because it only checks the locals_withut_parens and not import_deps.

zachallaun commented 1 month ago

I tried this branch, and it still adds parens to all the exunit macros. I think this is because it only checks the locals_withut_parens and not import_deps.

See my message above -- the ExUnit macros aren't included for whatever reason, I think we need to special-case some things. I'm going to look into how the Elixir formatter decided whether to put parens around calls.

Re: import_deps, they're expanded automatically into locals_without_parens. Here's what it looks like for a library of mine, for instance:

iex(3)> Mix.Tasks.Format.formatter_for_file("lib/mneme.ex")
{#Function<14.79746710/1 in Mix.Tasks.Format.find_formatter_for_file/2>,
 [
   sigils: [],
   inputs: ["{mix,.formatter}.exs", "{config,lib}/**/*.{ex,exs}", "test/**/*.{ex,exs}",
    "test_integration/**/*.{ex,exs}", "examples/*.{ex,exs}"],
   locals_without_parens: [
     all: :*,
     check: 1,
     check: 2,
     property: 1,
     property: 2,
     auto_assert: :*,
     auto_assert_raise: :*,
     auto_assert_receive: :*,
     auto_assert_received: :*
   ],
   export: [
     locals_without_parens: [
       auto_assert: :*,
       auto_assert_raise: :*,
       auto_assert_receive: :*,
       auto_assert_received: :*
     ]
   ],
   import_deps: [:stream_data],
   plugins: [Styler],
   line_length: 98
 ]}

all, check and property come from :stream_data.

zachallaun commented 1 month ago

Looks like the formatter special-cases a bunch of things. We should likely copy this list and can set parens?: false for those.

zachallaun commented 1 month ago

@scohen Still need to add more tests but give things a try now.

scohen commented 1 month ago

@zachallaun much better. This is good to go when the tests are done.

zachallaun commented 1 month ago

@scohen Pushed some additional tests. Are there other cases you'd like to see?