Ruby inline block conversion assumes conversion as hash

[ressu]

If one tries to convert a single statement inline block into a multiline block, the conversion assumes the inline block is a hash.

Original:

this { block gets, horribly: broken }

After :SplitjoinSplit

this {
  horribly: broken,
}

It's not too easy to detect these, but one good idea would be to test if the {} is preceded with a word, then it's most likely a block. If there is a = or no word, then it's most likely a hash. I bet there are better ways to detect this, but I can't think of them at the moment.

[AndrewRadev]

It really is difficult. The test for "starts with a word or )" should actually work, it doesn't seem like it's syntactically possible to have a hash there. I've just pushed a branch called better-ruby-block-detection, which implements this functionality (and includes your test case).

Unfortunately, there's still an issue. Given the following example:

this { block gets, horribly: broken }

If you split while on the block, you get:

this do
  block doesnt, get: mangled
end

Which is correct. However, if you split with the cursor on this, you still get:

this {
  get: mangled,
}

Which is the broken case.

The question here is "where should you be when you're splitting?". For most use cases, the cursor needs to be "inside" the structure you're splitting -- inside the hash's curly braces, inside the string's quotes, etc. For some cases, I try to be a bit more loose. This is useful for ruby method options in particular:

method_call one: 'two', three: 'four'

It feels much more intuitive to split on method_call than to go inside the options area, especially if you end up with the cursor on a string (since strings also get split into heredocs).

If I'm to do this "right", then it would make sense to either require you to be in the area to split in both cases, or allow you to be on the method call in both cases. Both are kind of tricky to do, mostly due to complicated-ish logic. Which one do you think would make more sense? Do you have a different idea?

For now, it would help if you switch to this branch and use it for a while, and report any regressions, just so I'm aware of any other limitations of this approach. I'll try to invest some effort next weekend, but it may take a while, I think it's about time to do some restructuring of the code anyway.

[ressu]

I would think that most of us are inside the block when doing a join or split, so that would be the logical direction here. I'll switch to the branch and see if I run in to problems.

[ressu]

Looks like everything works as planned. I it even manages to handle a difficult case of:

let(:var) { { has: hash, elements: too } }

[AndrewRadev]

Hmm, although this particular case seems wrong to me. It ends up as:

let(:var) { {
  has:      hash,
  elements: too,
} }

That's probably because it splits the hash inside the block and not the block itself. This is still okay, but if the cursor is on the first {, I think it should split to:

let(:var) do
  { has: hash, elements: too, }
end

I'll experiment.

[AndrewRadev]

I've pushed some more commits to make hash/option splitting and block splitting work the same: only when in the actual structure. The example you gave above now works as I specified -- splits the block first, then the hash. What do you think about this? I know that a lot of people prefer lets to have curly-brace blocks, even when multiline statements, but I can't think of a good way to give special handling to such cases.

[AndrewRadev]

As a side note, I'm going to have to take a second look at string splitting, it doesn't seem to work very well in some edge cases. Not sure if it's related to these changes or it was like that before, but it would be a good idea to debug while I'm at it.

[ressu]

Yup, the example case I mentioned was split on the first { the second case is also correct, despite looking a bit silly. If you wish to split a hash into multiline hash, then the code that is around that multiline hash shouldn't really matter.

If the code outside of the multiline hash is evaluated and handled, then the script is actually doing more than it should be doing.

[AndrewRadev]

One way to fix this would be simply to merge the block and hash handling callback and take an informed decision based on the case. This shouldn't be too difficult to pull off and might be a good idea in general, due to how coupled these two pieces of logic are.

That said, I'm not convinced I should make this change in functionality

Consider this example:

foo = { one: 'two', three: 'four' } if condition?

When I split this line, regardless of where I am in it, I expect that the result is this:

if condition?
  foo = { one: 'two', three: 'four' }
end

Afterwards, I can go down one line to the hash and split that one. What you're proposing is a general behaviour that goes like this:

foo = {
  one: 'two',
  three: 'four'
} if condition?

While this is syntactically valid, it's not very idiomatic. Not to mention I think the if-clause at the bottom impairs redability a lot, depending on how large the hash is, but that may be debatable.

From what I'm getting, you're saying the plugin should always limit the effect on areas around the cursor, but that's not how it was built to work. It's meant to turn single-line code into multiline code. For a lot of cases, there are more or less clear priorities and the plugin attempts to split along those.

The shorthand if-clause is one example of this. The only reason you would ever use it is if your line is short enough that it makes sense. If you want to split the line, it's likely due to it being too long and difficult to read. The suffix if-clause should always be the first to be transformed into a multiline if in that case. I would argue the same for blocks -- the single-line form of a block is a convenient shorthand, but if the line is too long, the block form should be the first to be changed.

The only reason that locality of the cursor is respected is due to the fact that, in some cases, you may have multiple non-intersecting segments that can be split. In that case, there are multiple reasonable ways to shorten the code and it would be inconvenient for the plugin to allow one, but not the others. Priorities don't help. With nesting, however, I don't think that's the case. First you split the outer part, then you split the inner. That's what I'd see as a logical series of steps towards reformatting the line. Forcing the plugin to always limit itself to the perfect definition around the cursor seems counter-productive. The let example may be correct, but it is silly :). The only reason to format your code like this is if you insist on keeping { braces on multiline let clauses due to your coding style, but this is something very specific that splitjoin probably couldn't handle.

What do you think?

[ressu]

Hmm.. you are right that the use case here actually should look at the big picture. The only concern I'm having here is that if we try to make too many assumptions for the user, it's going to backfire, since not everyone wants to use the code the same way.

One way of making the behavior controllable by the user, would be to allow visual blocks. That way the user could do va{gS and get the latter result, even if it doesn't make sense to us. That way both use cases would work the way the user might expect it.

[AndrewRadev]

The only concern I'm having here is that if we try to make too many
assumptions for the user, it's going to backfire, since not everyone wants to
use the code the same way.

True, and I think the only way is to stick to mostly idiomatic code and decide edge cases on a case-by-case basis. A few things also have options for code style, like curly braces, spacing between brackets and so on. Hopefully, if people use a coding style that's unsupported, they'd open an issue and I can decide whether it makes sense to support or not. If it's not supported, I guess it'll just have to be handled manually by users. Technically, they could even override the callbacks and write their own splitter/joiner, though this would require some vimscript knowledge.

One way of making the behavior controllable by the user, would be to allow visual blocks.

That's a very interesting idea! If I could allow splitting within a visual block then edge cases would be much easier to handle. Even joining by visual mode might make sense in some weird cases.

Unfortunately, I can't see a good implementation of this at the moment :/. Every split/join callback has a lot of power to investigate the buffer as it sees fit. Which is necessary, since there are checks for syntax (comments, strings) and uses of searchpair() and the likes. As a side note, I really wish Vim could give you the API for syntax highlighting and text jumping within normal strings somehow. Like, creating a "rich string" that mirrors the API of a buffer. But I'm assuming it would be a hard sell.

I do see one way of this working. Put the contents in a temporary, hidden buffer, perform a normal splitjoin on that, and then put it back. This may seem dirty, but it might just be crazy enough to work (I think this is how Gundo works, for instance). In any case, it shouldn't be difficult to try, so what the heck. I'll try to devote some time to the experiment these days.

[AndrewRadev]

So, it's been a while. Does the better-ruby-block-detection branch work well in your everyday usage? I'd like to merge it to master to start on the other issues.

The visual mode stuff seems quite doable, by the way. I just have to figure out a good way to reuse the code. Thanks for the idea :).

[ressu]

I've been using better-ruby-block-detection branch as my main version of the script for a while now. So far I haven't seen any ill effects and things have been quite smooth. So I'd say it's safe to merge.

[AndrewRadev]

Great, thanks. I've just merged it into master. I'll try to get to your other issue soonish. You can also watch issue #32 if you're interested in the visual mode logic.

[fuadsaud]

would it be possible to choose to keep the {} in multiline blocks instead of do...end?

[ressu]

I think it's generally recommended not to use {...} for multiline blocks. That being said, recommendation doesn't mean that it shouldn't be possible.

[fuadsaud]

I understand your point, but I beg to differ:
rubocop/ruby-style-guide#162
On Sat, Feb 27, 2016 at 6:06 AM Sami Haahtinen [email protected]
wrote:

I think it's generally recommended not to use {...} for multiline blocks.
That being said, recommendation doesn't mean that it shouldn't be possible.

—
Reply to this email directly or view it on GitHub
#30 (comment)
.

Fuad Saud

http://fuad.im • https://www.linkedin.com/in/fuadsaud

[AndrewRadev]

Regardless of whether it's recommended or not, it probably won't be an issue to implement a setting for it. I'll take a look at it one of these days and see if I can add one.

[AndrewRadev]

@fuadsaud I've just pushed a commit that adds a setting, splitjoin_ruby_do_block_split, which should fix your issue. Something like this:

let g:splitjoin_ruby_do_block_split = 0

Take a look at the documentation for more details.

[fuadsaud]

@AndrewRadev that's utter cool! works perfectly, thank you!