Basic implementation for Elixir pipelines #179

jonleighton · 2021-09-26T02:30:31Z

It has known issues with handling multi-line arguments, and solving it
seems quite complicated, so I thought this might be sufficient for an
initial implementation.

It has known issues with handling multi-line arguments, and solving it seems quite complicated, so I thought this might be sufficient for an initial implementation.

jonleighton · 2021-09-26T02:32:41Z

I'm not very good at vimscript, or familiar with the implementation of splitjoin.vim, so let me know if you have feedback.

I tested a few examples on a real-world project and the main issue I came up against was the handling of multi-line arguments. I'm not really sure how to deal with this, or even how to detect the scenario so that we can exit without making any changes.

As far as I can tell, sj#argparser#elixir#LocateFunction() currently only handles function calls that are all on the same line, but I might be wrong about that?

AndrewRadev

Thank you for the work you put in -- I'm particularly happy about the tests, which managed to catch at least one issue with a change I made to the PR.

In general, it looks good, and I'll be happy to merge it -- I've added a few notes and suggestions for changes.

In terms of one-line vs multiline, you're right -- when splitting, the plugin needs the starting point to be clear from a single line, leading to the function call splitter doing the same. As a whole, multiple lines tend to complicate things a lot, because there's many ways to split the same structure and it can be hard to decide what to do if you have more context than just one line to work with.

My sideways plugin is significantly better at this, because it's limited in a different way. Sideways works only on list-like items, and it needs to be able to find a clear beginning of a list, but it happily goes backwards over multiple lines to detect it (though it did take a while to get it to work :)). Lists tend to be non-overlapping. You could have one type of list in another type of list, but you can detect what the "closest" one is.

In fact, I'm not sure if splitting arguments into expressions like this is going to work reliably with this plugin. Consider Enum.join(["split", "join"]) -- if we trigger a split, do we trigger the pipe thing, or do we put "split" and "join" on separate lines like in your example? Right now, this would work -- with the cursor on [, it splits the array, and with the cursor on join, it pulls out the argument. Same if it were just function_call("foo", "bar") -- it might do different things depending on where it is, although I'm not sure which would be more intuitive -- cursor on "foo" pulls it out with a pipe, and cursor on function_call( splits the arguments? Or should it be the other way around?

In the end, the ambiguity comes from one button having to do multiple things depending on context, which is convenient, but only until it's intuitive, then it starts to be a problem. Callbacks can be turned on and off, so this is a fixable problem, of course. A general solution for pipes in particular might involve sideways instead and a separate keybinding, since I'm not 100% sure if pipes would feel right for "splitting". I have no problem merging the PR, though, just throwing it out as a warning.

If you wanted to experiment with sideways, I imagine you'd start by getting the arguments with this function: autoload/sideways/parsing.vim. Here's the code that uses the output of that function to operate on arguments: autoload/sideways/textobj.vim. You'd have to take the text of the first arg based on start + end position, delete it from the buffer, maybe the way that s:MarkCols does it, with an "_d and then insert the text above with a |>, deciding how to format the multi-line content.

AndrewRadev · 2021-09-26T08:54:14Z

autoload/sj/elixir.vim

+  let args = parser.args
+
+  let function_call = sj#Trim(strpart(line, 0, function_start - 2))
+  let result = args[0] . "\n|> " . function_call . '(' . join(args[1:], ', ') . ')'


One thing that seems to be a problem is that if the function doesn't end at the line, some content is lost, for example:

foo("one") + bar("two") "one" |> foo()

What you could do is take the remainder of the line and put it back where it was:

Suggested change

let result = args[0] . "\n|> " . function_call . '(' . join(args[1:], ', ') . ')'

if function_end > 0

let rest = strpart(line, function_end + 1)

else

let rest = ''

endif

let result = args[0] . "\n|> " . function_call . '(' . join(args[1:], ', ') . ')'.rest

Mind you, I'm not actually sure how operators are associated in elixir -- is the + going to be okay with this transformation? You could also enforce that the rest of the content is non-whitespace, !~ '^\s*$, just return 0, to ensure that you're always splitting just one function call on a line.

Trying this out with String.length() rather than foo(), I get:

"one" |> String.length() + String.length("two")

Output:

** (ArgumentError) cannot pipe "one" into String.length() + String.length("two"), the :+ operator can only take one argument (elixir 1.11.2) lib/macro.ex:290: Macro.pipe/3 (stdlib 3.14) lists.erl:1267: :lists.foldl/3 (elixir 1.11.2) expanding macro: Kernel.|>/2 test.exs:2: (file)

The following works (and that's how mix format formats it):

("one" |> String.length()) + String.length("two")

But I've no idea why anyone would want to do that!

I think we're better off trying to detect this scenario and refusing to do anything? It doesn't really seem like an important use case to support.

Yeah, agreed 👍

AndrewRadev · 2021-09-26T09:01:03Z

autoload/sj/elixir.vim

+
+  if line =~ '^\s*|>\s\+'
+    return 0
+  endif


Here's one example I have laying around that doesn't work well, it seems:

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

Splitting with the cursor on the where gives me:

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

It could be argued that people should just not attempt this, but I try to add safeguards if I can. Do you think it makes sense to add another check here like:

if line =~ '^\s*\k\+:' " Starts with a `keyword:`, part of a dictionary return 0 endif

Comment optional, and you could consider joining the pattern to the one above, too.

Right, good catch. I didn't think much about single-line pipelines, although maybe there's a case for supporting them somehow.

I guess it would be ambiguous how I'd want to split this:

String.length("foo")

I.e. should it be:

"foo" |> String.length()

Or this:

"foo" |> String.length()

But at least the joining case is less ambiguous. It seems reasonable that this:

IO.inspect("foo" |> String.length())

Could join to this:

IO.inspect(String.length("foo"))

And in fact split to this, depending on cursor position:

"foo" |> String.length() |> IO.inspect()

In any case, I think for now I'll have a think about how we can ignore these situations as I reckon it'll probably be a bit of a can of worms that I'm not keen to open now. (But might after getting some use out of these changes and seeing whether I miss the above functionality in the real world...)

AndrewRadev · 2021-09-26T09:15:22Z

autoload/sj/elixir.vim

+  endif
+
+  call sj#PushCursor()
+  normal! 0f(


This is tricky. Since a function doesn't have to have round brackets, this could jump to the wrong place. Consider this:

foo "one" + bar("two")

I kind of understand why you go to the start of the line. In a foo("one") + bar("two") situation, you don't want to pull out the "two" of bar, since it would be syntactically incorrect -- you need the leftmost function call. But in this case, you won't get it, since jumping with f( puts you on the bar(:

"two" |> foo "one" + bar()

Maybe something like this instead, to jump to the end of the closest function-looking thing after the start of the line?

Suggested change

normal! 0f(

normal! 0

if search('\k\+[!?]\=[ (].', 'We', line('.')) <= 0

return 0

endif

The search function can be difficult to read, this particular combination of flags makes it jump to the end of the pattern, limited to the current line: :help search()

As discussed above, I think with this example there's no a good way to split it into a pipeline (with or without parens on the function call). So I'd rather just find a way to ignore it altogether and focus only on calls that occupy the entire line, if you're happy with that?

Sure, that makes sense -- I would consider comments, like foo("bar") # baz, but the comment being removed might be fine.

Well, in this case, foo "one" + bar("two"), the single argument to foo would be "one" + bar("two"), but I suppose transforming it into "one" + bar("two") |> foo would be incorrect without round brackets around the pre-pipe expression.

jonleighton · 2021-09-27T11:15:16Z

@AndrewRadev Thanks for the thorough and fast review!

Some responses:

In fact, I'm not sure if splitting arguments into expressions like this is going to work reliably with this plugin. Consider Enum.join(["split", "join"]) -- if we trigger a split, do we trigger the pipe thing, or do we put "split" and "join" on separate lines like in your example? Right now, this would work -- with the cursor on [, it splits the array, and with the cursor on join, it pulls out the argument.

Yeah, that's a good point and I see what you mean that the "right" behaviour is inherently ambiguous.

Same if it were just function_call("foo", "bar") -- it might do different things depending on where it is, although I'm not sure which would be more intuitive -- cursor on "foo" pulls it out with a pipe, and cursor on function_call( splits the arguments? Or should it be the other way around?

I don't think the splitjoin.vim currently has the functionality to split function args onto multiple lines? But yeah, I can see that perhaps some users would desire it.

That said, mix format seems to be fairly popular in the Elixir world and takes some of these decisions away from the programmer. But there are still some cases where I'll find myself intentionally splitting a single line into multiple lines (and the formatter lets me do that). In those cases I tend to do f,a<CR> and then save, which is enough to trigger the formatter to wrap onto multiple lines.

sideways.vim looks handy, so thanks for the link! I hadn't come across it before. I agree with you that perhaps it makes more sense for this, although I'm not too sure if there is a use case for manipulating pipelines except for at the beginning (i.e. the first |>). I'll ponder this and consider making a PR there, although I suspect I won't get around to it soon.

I'll reply to your other suggestions inline, and I'll come back to make some changes during the next week.

AndrewRadev · 2021-09-27T13:51:00Z

I don't think the splitjoin.vim currently has the functionality to split function args onto multiple lines? But yeah, I can see that perhaps some users would desire it.

Yeah, it's not implemented at this time, though most languages have it, and I might implement it on request.

I'm not too sure if there is a use case for manipulating pipelines except for at the beginning (i.e. the first |>)

It's less about picking an argument than it is about pulling the first argument regardless of how things are spread over multiple lines. The function I referenced gives you the start and end coordinates of all the list items in the current list, not just the ones your cursor is on, so you can always operate on the first one.

* Handle multiple calls on one line (by doing nothing) * Handle lines ending with a comment The function call parser is not able to determine the end of calls that don’t use parentheses. It returns a function_end of -1 in those cases. However we can parse out the comment to deal with it. Though this would break down if we have a ‘#’ inside a string, like this: foo "bar#baz" # bla I think this is probably good enough for now.

Otherwise we’re likely to end up with scenarios where the “right” behaviour is very hard to determine.

This already works

jonleighton · 2021-09-30T07:14:16Z

@AndrewRadev I think I've addressed all your comments with the above commits, let me know what you think.

AndrewRadev · 2021-09-30T17:55:15Z

Looks good to me -- I'll go ahead and merge this, thanks for your work 👍. Happy to review fixes and adjustments as you use it in practice.

jonleighton · 2021-09-30T23:46:04Z

Thanks! 🙌

Basic implementation for Elixir pipelines

c440a3a

It has known issues with handling multi-line arguments, and solving it seems quite complicated, so I thought this might be sufficient for an initial implementation.

AndrewRadev requested changes Sep 26, 2021

View reviewed changes

jonleighton added 4 commits September 30, 2021 16:30

Only split function calls at start of line

47584b5

Otherwise we’re likely to end up with scenarios where the “right” behaviour is very hard to determine.

Add explicit test for pipe in call args

3d1aec3

This already works

Add explicit test for multiple no-parens calls

0cf6f77

AndrewRadev merged commit 65efd9d into AndrewRadev:main Sep 30, 2021

-  let result = args[0] . "\n|> " . function_call . '(' . join(args[1:], ', ') . ')'
+  if function_end > 0
+    let rest = strpart(line, function_end + 1)
+  else
+    let rest = ''
+  endif
+  let result = args[0] . "\n|> " . function_call . '(' . join(args[1:], ', ') . ')'.rest

Basic implementation for Elixir pipelines #179

Basic implementation for Elixir pipelines #179

Uh oh!

Conversation

jonleighton commented Sep 26, 2021

Uh oh!

jonleighton commented Sep 26, 2021

Uh oh!

AndrewRadev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonleighton commented Sep 27, 2021

Uh oh!

AndrewRadev commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonleighton commented Sep 30, 2021

Uh oh!

AndrewRadev commented Sep 30, 2021

Uh oh!

jonleighton commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AndrewRadev left a comment •

edited

Loading

AndrewRadev commented Sep 27, 2021 •

edited

Loading