AndrewRadev/splitjoin.vim

Elixir bug

LandonSchropp opened this issue ยท 15 comments

I'm having an issue splitting this function syntax in Elixir:

let(:authenticated_conn, do: unauthenticated_conn() |> authenticate(user()))

This should split to the following:

let(:authenticated_conn) do
  unauthenticated_conn() |> authenticate(user())
end

Instead, splitjoin.vim outputs this (notice the parentheses placement):

let(:authenticated_conn do
  unauthenticated_conn() |> authenticate(user()))
end

Thanks!

This should also work in reverse. This function:

let(:authenticated_conn) do
  unauthenticated_conn() |> authenticate(user())
end

Should be joined to this:

let(:authenticated_conn, do: unauthenticated_conn() |> authenticate(user()))

I've started to do some work on this in the branch elixir-improvements, but I'm hitting some failing tests with method definitions. I don't write elixir myself, so I'm not very familiar with the syntax, but do blocks for method definitions at least seem to work like this:

defmodule Foo do
  def bar(), do: :bar
end

Which is different in that the do: sits outside of the brackets with a comma, but I'm guessing this wouldn't work for the let case? I did some googling, but I'm still kind of unclear on the details, especially with examples like this one:

defmodule Test do
  def test(opts \\ [], do: block) do
    IO.inspect(block)
  end
end

Two do-blocks raises questions like how would this get joined ๐Ÿ˜….

Could you explain what the scenarios are for do-blocks in elixir, or maybe link to the right documentation page? Can the , do: sit both inside and outside the round brackets, or is the outside a special case for defs?

Thanks for taking a look at this @AndrewRadev! I'm fairly new to elixir myself, so what I'm saying could be wrong, but my understanding is the "do blocks" in Elixir are really just syntactic sugar for keyword lists), and many of the built-in expressions are really just macros. For example, an if "statement" like this:

if foo do
  bar
else
  baz
end

Is really just syntactic sugar for a function:

if(foo, do: bar, else: baz)

This article dives into what keyword lists are and why their syntax looks the way it does.

The let example I gave is a function macro defined by the ESPec library, but it's really just a regular function, so you're right that the do: syntax should be inside the parentheses.

As for the def example you gave, that one has me stumped. I'll reach out to the Elixir Slack group and see if anything with more Elixir experience is willing to chime in and help.

Thanks again!

Well, technically

def foo(do: block) do
  # โ€ฆ
end

is

def(foo(do: block)) do
  # โ€ฆ
end

Which is:

def(foo(do: block), do: โ€ฆ)

Of course no-one is using the latter forms, but technically these are equivalent due to fact that def is a macro, just as if. However I think it would be near to impossible to implement it with current, regex based, implementation of splitJoin, as it would require full fledged parser to understand this magic.

@hauleth Thanks for the reply! Do you think there's a pragmatic implementation of this based on mix format? Since that's the standard formatting for Elixir, it feels like that might be good enough for most situations. I think the following syntax and cases would cover the major use cases:

Function Definitions:

def full_name(first_name, last_name) do
  "#{ first_name } #{last_name}"
end

def full_name(first_name, last_name), do: "#{first_name} #{last_name}"

If/Else:

if foo do
  bar
else
  baz
end

if(foo, do: bar, else: baz)

Other Dos:

let(:banana) do
  "banana"
end

let(:banana, do: "banana")

What do you think?

Of course no-one is using the latter forms

I think this is key, I feel that the important thing is to consider what is commonly used, more than what is technically possible. Landon's summary looks sensible to me, I'll work in that direction.

Though, one thing I'd like to confirm is that, if I'm not mistaken, all of these have optional brackets, right? Could if(foo, do: bar, else: baz) be written if foo, do: bar, else: baz? The logic in the branch works with spaces as well, at least for joining, but if there's clear conventions about when to use them, that could simplify things.

Yep, you're correct. Both ifs are valid, and mix format doesn't have a preference for which syntax is used. Anecdotally, it seems like people lean towards the second version for if.

It can be problematic with code like:

for a <- 1..10,
   Integer.is_odd(a) do
  a
end

Which is the same as:

for a <- 1..10,
   Integer.is_odd(a),
   do: a

Not:

for a <- 1..10,
   Integer.is_odd(a, do: a)

To clarify my earlier point, I would expect the split to remove the parentheses from if.

if(true, :foo, :bar)

Should become:

if true do 
  :foo 
else
  :bar
end

Okay, I've done some work in elixir-improvements and I'd appreciate it if you could try it out and let me know how it feels and what edge cases you can find.

@LandonSchropp All of your "do"-s, including removing brackets from the if-clauses, should work. Though, I've gotten them to work for if(true, do: :foo, else: :bar), and in the last example you've put if(true, :foo, :bar) -- is that a shorthand form that is also a valid if? (I should really just set up a working elixir environment and start running code)

@hauleth Yes, that's true, in two different ways. One is that the plugin only looks on the current line for a structure to work on, so it wouldn't work on this:

for a <- 1..10,
  Integer.is_odd(a) do
  a
end

Working on definitions split across multiple lines is... logistically complicated. I'd rather consider it an inherent limitation of the plugin, although if you think it's a common case, I could try to put in some work into joining the above on the for keyword into:

for a <- 1..10, Integer.is_odd(a) do
  a
end

With this structure, putting the cursor on the for line and joining results in:

for a <- 1..10, Integer.is_odd(a), do: a

However, there's a catch -- putting the cursor inside the Integer.is_odd( ... ) does result in the do block being joined inside. The plugin works by searching backwards for a "beginning of a function call", and so it sees Integer.is_odd(a) do and figures this is it.

This also happens for do blocks in def calls:

def test(opts \\ [], do: block) do
  IO.inspect(block)
end

Joining on def test works correctly:

def test(opts \\ [], do: block), do: IO.inspect(block)

Joining with the cursor inside the test(...) joins to

def test(opts \\ [], do: block, do: IO.inspect(block))

So that's not ideal, but at least it's predictable by where your cursor is. I don't know, I'll think about it -- for defs in particular, I could maybe add a special case.

Awesome @AndrewRadev! I'll give this a shot over the next few days and let you know how it works out. Thanks for tackling this!

is that a shorthand form that is also a valid if?

Nope, that doesn't compile.

If you'd like to try out Elixir, getting it up and running is really easy. There are instructions here. It's a fun language to useโ€”I'm liking it so far. ๐Ÿ™‚

Elixir uses ex and exs extensions. If you want to test out some code, you can create a small script and run it like this: elixir example.exs. You can also use the interactive reply iEx but running iex in your terminal, which is handy for seeing if syntax is valid.

Thanks again!

I've had a chance to check this out, and it's looking great. I found two small issues, neither of which really concerns me. However, I thought I'd mention them to you in case you cared.

The first is that the indentation after splitting seems to be a little off:

if true do
:foo
else
:bar
end

Should be:

if true do
  :foo
else
  :bar
end

I'm not worried about this because mix format automatically fixes it for me, and I have Vim set to run mix format on save.

The second issue is that mix format will automatically split up lines when they're too long like this:

if Enum.member?(unquote(schema).__schema__(:fields), :deleted_at),
  do: query |> where([schema], is_nil(schema.deleted_at)),
  else: query

SplitJoin will not split those lines. However, that's easy to do by combining them with J, and if I want to split it after that SplitJoin works just fine.

Thanks again for tackling this! It definitely makes my life easier. ๐Ÿ™‚

Thanks again for tackling this! It definitely makes my life easier. slightly_smiling_face

Glad to hear it :)

The second issue is that mix format will automatically split up lines when they're too long like this:

I've added another join callback to that branch -- with the cursor on the if-line in your example, a splitjoin-join should correctly join both of these lines, allowing you to split them into an if-else afterwards. Let me know if you think that makes sense or if you run into issues -- if it turns out there's side effects, I might remove it.

The first is that the indentation after splitting seems to be a little off:

I'm not sure why this would be happening -- for me, indentation seems to be applied correctly. The way the plugin works, it essentially executes a = operator on what it had replaced (not always, but it should be doing it in this case). Are you using https://github.com/elixir-editors/vim-elixir? Which git sha of the plugin is it?

For the first one, your new callback seems to work great! I think that approach makes perfect sense.

I'll bet my setup is to blame for the second one. I'm using LunarVim, which uses nvim-treesitter under the hood. Treesitter support is still somewhat experimental (I don't think it's supposed to be stable until Neovim 0.6), and I've noticed small issues with it as I've been using it. I'll bet there's a slight error in the Elixir treesitter indentation that's causing this.

Thanks again!

I've merged the branch, so everything should now be available normally -- I'll close this issue for now, but feel free to reopen if y'all run into issues, or open new ones.