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

split ruby blocks correctly when ; is inside string literal #199

Merged
merged 5 commits into from Mar 21, 2023

Conversation

rrhubenov
Copy link
Contributor

I was in the intro to Vim course in FMI when this bug was mentioned.
Decided this would be fun way to start my vimscript journey.

I'm not familiar with Ruby so I'm not sure if this handles all the edge cases.

The regex might be kind of overkill but I couldn't really find an easier alternative
to match only those ';' that are not surrounded by quotes.

Checking if ";" is NOT surrounded by quotation marks
<=>
NO quotation mark on its left OR NO quotation mark on its right

Would love some feedback!

Copy link
Owner

@AndrewRadev AndrewRadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for trying it out, it's a very good attempt :). There's some practical problems, and I'd recommend a different approach. One is that ' is not the only way to write strings in ruby -- using " wouldn't work:

foo { "example1; example2" }

foo do
  "example1
example2"
end

It's also possible to have strings with %() and stuff like that... we could figure out a way to handle it with regexes, but the method I would use is to check the syntax group under the cursor.

If you execute this expression with the cursor inside a string, you'll get rubyString: synIDattr(synID(line('.'),col('.'),1),'name'). This is the common method to search for things while skipping strings and comments. Here's a more verbose explanation of how it works: https://youtu.be/1AOhfnokacE?t=443

I've got a utility function in this project, sj#SkipSyntax, which generates a "skip expression" for searches. So if you have this code:

foo { "example1; example2"; foo }

And call echo sj#SearchSkip(';', sj#SkipSyntax(['rubyString']), 'W', line('.')) ('W' for "don't wrap around the buffer", line('.') for "stop at the current line"), it'll jump to the ; before foo and print 1.

The annoying thing is this has to be done in the buffer -- after taking the contents as a string, there's no more syntactic information. So there's two methods I was planning on trying out:

  • loop while the results of sj#SearchSkip are > 0, and store the column number of the found ; characters. Then, use these column numbers and the column number of the body to figure out where in the string to replace ; with a newline.
  • Replace the body, then move the cursor to the replaced line and do the loop then with a exe "normal! r\<cr>" or something.

Either one should work, I was probably going to try both, but I'm not in a hurry. If you'd like to give it a shot, try it out :).

@rrhubenov
Copy link
Contributor Author

Thank you!
I'd definitely like to try and figure this out, so I'll make some progress this week with the methods you suggested and
report back my results :). It's been quite fun so far

" remove leftover whitespace
let replacement = substitute(replacement, '\s*\n', '\n', 'g')

call sj#ReplaceMotion('Va{', replacement)

call search(replacement, 'cW')
normal! j0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndrewRadev
I implemented the second method you described.
But since search(replacement, 'cW') jumps one line above the body of the "do; end" block, I had to
use "normal! j0" to move to the beginning of the correct line.
It seems to me that moving 1 line lower is kind of dirty, but I'm not sure.

I could use sj#GetMotion('Vi{') but I suspect there might be a cleaner way.
Do you have any suggestions :)?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem in j0, you'll see me do this kind of thing elsewhere as well :). Nothing dirty about using the basic motions.

A practical problem, though, is searching for the replacement. What if the replacement has special characters, like *? You can try it with foo * bar, which as a pattern is "foo, followed by spaces, followed by bar", which won't actually match the string :). It's possible to search for a literal string by prefixing it with \V, but it feels unnecessary. Why the search? Where are you trying to position the cursor? Wouldn't just j0 be enough to get you in the right place? At least when I'm testing the code out, it seems like removing the search still works fine. Tests are passing too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, great!
I wasn't sure where ReplaceMotion would leave the cursor.

I've left only j0. Less code, less bugs :)

@rrhubenov
Copy link
Contributor Author

I decided to implement the second method since it seemed easier. I'll try and make an implementation of the first method
as well if I have time.

@AndrewRadev
Copy link
Owner

Thanks for the fix, I'll go ahead and merge it 👍

@AndrewRadev AndrewRadev merged commit 2c8cd19 into AndrewRadev:main Mar 21, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants