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

Incorrect Switching Ruby multiline block with comment and Ruby single Line block #122

Open
ceclinux opened this issue Nov 24, 2017 · 6 comments

Comments

@ceclinux
Copy link

ceclinux commented Nov 24, 2017

Incorrect switching between Ruby multiline block with comment and Ruby single Line block

Expected behavior

['a'].each do |t|
  # comment here
  p t
end

press gJ

# comment here;
['a'].each { |t|  p t }

or maybe tell the user switch failed

Steps to reproduce the problem

['a'].each do |t|
  # comment here
  p t
end

press gJ

['a'].each { |t| # comment here; p t }

Ruby doesn't have single line comment, that this conversion will raise a syntax error

@AndrewRadev
Copy link
Owner

Good catch, I try to handle comments when I can (in ruby in particular), but there was a subtle bug in this case. I've pushed a fix to master. Could you check it out and let me know if it works?

@ceclinux
Copy link
Author

sure 😃

@ceclinux
Copy link
Author

On second thought, give Splitjoin: Can't join this due to the inline comment(s). Please remove them first. would be a better solution, because the comment(s) in Ruby multiline blocks will always deviate from what they want to comment, for example:

[].each do |t|
  do first thing
  # comment here
  do second thing
  ...
end

press gJ(after this fix 2659bdf)

# comment here
[].each { |t| do first thing; do second thing; ... } 

the comment is intending for explaining second thing is doing before switching. However, after switching to single line block ,the comment is placed to comment the whole block.

@AndrewRadev
Copy link
Owner

AndrewRadev commented Nov 27, 2017

There's no perfect solution here, I think. This isn't a new issue: #31

My thinking is that, regardless of whether that comment is used to describe something in the middle, the important thing is to keep the comment. If it's no longer needed afterwards, it can easily be deleted, but if splitjoin deletes it, it'll be a bit harder to restore (undo, delete, splitjoin again, paste). And yes, I could just block joining if there's a comment, but, as you see from the issue, other people have different preferences :).

My personal preference would be just as yours -- I wouldn't even try to join a block that has a comment in it (or, for that matter, a block with more than one line). But then, if I don't try to join blocks like this, it doesn't matter to me if splitjoin tries to do it. Is it a problem for you? Do you accidentally join blocks like this and it gets in the way or something?

@ceclinux
Copy link
Author

Actually, I don't join the block often. I think the problem here is if someone wants to quickly refactor his/her code, I mean, who use gJ quickly. The misplaced comment may cause a problem because the code won't raise a Syntax error so he/she wouldn't even notice it. People who later maintain these codes will get confused by the comment.

Also, I believe inherently this plugin should have this property(reversible)

split(join(original code)) == original code

which is false if we keep the comment above the joined code

My opinion the join the Ruby block with comment feature should a config for this plugin and the default value is false

@AndrewRadev
Copy link
Owner

AndrewRadev commented Nov 27, 2017

Also, I believe inherently this plugin should have this property(reversible)

That's not true, though. I mean, it already doesn't. For starters, trailing commas can get added or removed, depending on settings. Whitespace gets normalized, due to the fact that, implementation-wise, the plugin looks for the pieces of whatever it's transforming and takes them regardless of delimiters.

There's a perl case that only works for splitting, because two different construct split into one thing, and then joining has to choose one or the other (and/or clauses splitting to if/unless). There's a python case (splitting var assignment), which splits into something that's, technically speaking, incorrect, because variable swapping is a bit special. I have a warning for that in the docs.

My point is, I'm mostly okay with this. I try to keep things consistent, but maintaining exact reversibility is, I believe, not particularly useful. It's always possible to undo, and I think that undoing is what a user of the plugin should do if the result is not what they expect, not try to join. A join might work, but undo will always work :).

Actually, I don't join the block often. I think the problem here is if someone wants to quickly refactor his/her code, I mean, who use gJ quickly.

I don't know if that's very likely, though. Splitting the code with a comment inside, your cursor will be literally on the newly-moved comment, because the plugin keeps the cursor position in place. I really don't think anybody would just split and somehow jump elsewhere and not even see what the result was. If nothing else, the whole point is you're currently editing this code, that's why you're reformatting it. Wouldn't you look at it after it's done and decide how to transform it further or something?

I'm not saying it's impossible -- maybe somebody is automating the process with a :global/.../normal! gJ or something. But any kind of automated or quick transformation should be checked afterwards for mistakes, I think. Easy to undo, tweak things, and then reapply, easy to change the comment later.

I guess there's a risk that somebody would ignore the comment, but I don't know. It's a difficult thing to predict, and I'd rather not try to account for every single possibility. Maybe if somebody came in with a use case, like "well, I ran this in a script, so I didn't know..." I could consider it, but I'd rather not add more complexity to the code for something that, to me, seems unlikely to be a practical problem. What do you think?

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