Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruby: Splitting a block requires cursor on opening brace #127

Closed
jonahx opened this issue Feb 17, 2018 · 3 comments
Closed

Ruby: Splitting a block requires cursor on opening brace #127

jonahx opened this issue Feb 17, 2018 · 3 comments

Comments

@jonahx
Copy link

jonahx commented Feb 17, 2018

If I type gS with my cursor on the block-containing I want to split, but not on the opening brace itself, nothing happens:

image

However I move my cursor to the opening brace:

image

and the press gS it works as expected:

image

Not clear from the docs if this is expected behavior or not, but in any case it would be nice if I could invoke the split shortcut gS from anywhere on the line I'm targeting.

Thanks. I love this plugin.

@AndrewRadev
Copy link
Owner

Thanks. I love this plugin.

Thank you for the kind words :)

Regarding the docs, there is a sort-of explanation here:

In general, if you're trying to split a structure, try to "be inside" when you
do so. This doesn't make sense in cases like the "if" statement, but it does
for hashes.
. Admittedly, there's no item-by-item warning as to where you need to be to split something. I feel like it might make sense to go through each case, but it's... a lot of effort. I'll consider doing it.

There are two general problems with this. One is priorities. The list of functions that try to split an area of code is here:

\ 'sj#ruby#SplitModuleNamespace',
\ 'sj#ruby#SplitArray',
\ 'sj#ruby#SplitArrayLiteral',
\ 'sj#ruby#SplitProcShorthand',
\ 'sj#ruby#SplitBlock',
\ 'sj#ruby#SplitIfClause',
\ 'sj#ruby#SplitCachingConstruct',
\ 'sj#ruby#SplitWhenThen',
\ 'sj#ruby#SplitCase',
\ 'sj#ruby#SplitTernaryClause',
\ 'sj#ruby#SplitOptions',
\ 'sj#ruby#SplitString',
(for ruby in particular). The functions are executed in order, so, for instance, if "SplitBlock" succeeds in splitting, the next one, "SplitIfClause", won't be called.

Since if-clause splitting is global, this allows someone to split in two different ways depending on where the cursor is:

foos.each { |f| puts f } if bar?

# Cursor on the start of the line:
if bar?
  foos.each { |f| puts f }
end

# Cursor in the block
foos.each do |f|
  puts f
end if bar?

Now, I'd argue that the second one is mostly a bad idea, maybe. In fact, I've made the argument elsewhere in the issues that the plugin is meant for use with refactoring, and if you're going to be splitting a hash/block with a trailing if-clause, you'd better split the if-clause first, always.

This does bring me to point two, though: Regardless of whether it's a good idea or not, it's complicated to get this stuff to work "correctly" for many cases :/. Having splitters that work on the entirety of the line means the only difference is priority, and that can be iffy. Limiting the area of effect to "the insides of {}-brackets" makes it less likely that splitting a block or hash would interfere with something else.

(In that line of thought, I've recently fixed a ruby curly-brace ambiguity, just so you're aware of this kind of trickiness: #124. You could try the branch I created in that issue, too, if you like the feature)

Anyway, I'm not saying it's impossible, it's just that it can be difficult to fix all the possible side effects that might happen. I do have tests for this, but getting them to pass might be a challenge. I tried making some naive changes in that direction, and there's definitely a lot of breakage.

Is this targeting a big inconvenience to you? Do you think if I made it clearer in the docs which splitters work on the line and which work on areas, it would be easier to orient yourself and get used to it?

@jonahx
Copy link
Author

jonahx commented Feb 25, 2018

Thanks for the detailed explanation. Interesting problems. I'd file this as a minor annoyance, so if it's a lot of work to fix, no big deal. That said, if you pursue this at some point, here would be my vote:

Having splitters that work on the entirety of the line means the only difference is priority, and that can be iffy.

I think the current behavior is perfectly reasonable for the example you gave -- when you have a block and an if on the same line. You could even show a vim message in that case explaining the ambiguity.

However, if there was not such an ambiguity -- ie, if the line only had a block, or only had an if -- then it should work no matter where the cursor is. And I think this is like the 95% case.

Is this targeting a big inconvenience to you? Do you think if I made it clearer in the docs which splitters work on the line and which work on areas, it would be easier to orient yourself and get used to it?

Not worth the trouble, imo.

@AndrewRadev
Copy link
Owner

However, if there was not such an ambiguity -- ie, if the line only had a block, or only had an if -- then it should work no matter where the cursor is. And I think this is like the 95% case.

Sure, I get that. It would be pretty much impossible to achieve with the current architecture though. There would have to be some sort of second pass that calls different callbacks that work on a line instead of on an area (the area thing is still necesary -- think two hashes on one line, as args to a function or something). Or try to find a target on the line... Can't imagine this without rewriting a lot of the plugin.

I'd rather be clear about the way it works and and expect people to jump to the target areas. Vim makes this reasonably easy, I think, even if it is a few keystrokes more.

Anyway, I'll close this issue as a wontfix. I appreciate you opening it anyway, it's a discussion I can reference next time there's a similar question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants