Adding R language support for function call split/join #172
Conversation
These are only used for R, so ideally, they should only be defined for this file.
There was a problem hiding this comment.
Thank you very much for your work! You've put in a lot of effort into getting this done.
I've taken the liberty to push some tests, and also to reformat the code based on my preferences. You can see the individual commits for the details, but it boils down to:
- Helper methods defined as script-local functions, so the file doesn't define anything "external" other than the two entry points (there are exceptions for this around the codebase, but only when I've needed to reuse the functions)
- Public entry points at the top, helpers below
- Some provided functions have been used instead of reimplementing them, like
sj#GetByPosition.
I've also left some comments around the code that you can take a look at, but I won't require you to take action on them -- just something to consider.
What I'd like is if you could also add some documentation in doc/splitjoin.txt. The sections are sorted case-insensitively, so the "R" section should be after "Python" and before "Ruby". If you have problems doing that, or are not sure about formatting, etc, I can tweak it afterwards.
|
|
||
| call s:MoveCursor(cursory, max([cursorx-1, 0])) | ||
| call sj#PushCursor() | ||
| endfunction |
There was a problem hiding this comment.
It's an interesting approach, and it does seem to work reliably for these cases. Still, I've left it as a private/script-local function, because it's not used anywhere else, and I'd really like to have multiple use cases to decide what a sensible interface for it could be.
For instance, neither the mask and inserts are being used right now. Is an array of booleans a useful interface for mask? How are inserts going to be figured out, since they're not used at all right now? I would avoid these extra parameters until they're actually used in practice.
But as a private function, this is fine -- if and when I end up using it for something else (or get a PR using it), it might be a good point to move it to the shared sj.vim file.
There was a problem hiding this comment.
Ah, apologies for letting those slip through. I had used them at one point in the implementation and had forgotten to go back through and clean up the interfaces.
The intention was that mask could be used to indicate when a part of the replacement draws its origin from the original text. For some splits/joins the content of the code changes, so it's necessary to mask portions out from being used to try to align the cursor. inserts was used in conjunction with this to specify how many characters need to be inserted between lines to properly align the cursor. An example might be:
let x = len(a) ? "true" : "false"
" after split, becomes:
if len(a)
let x = "true"
else
let x = "false"
endifI never settled on something that I was particularly happy with to accommodate more comprehensive rearrangements and ended up scrapping the attempts.
I'll clear these vestigial bits out and leave it for another exercise to try to handle non-whitespace or non-sequential rearrangments.
| specify "function calls with align_args = 0" do | ||
| vim.command('let g:r_indent_align_args = 0') | ||
|
|
||
| set_file_contents 'print(1, a = 2, 3)' | ||
| vim.search('1,') | ||
| split | ||
|
|
||
| assert_file_contents <<~EOF | ||
| print( | ||
| 1, | ||
| a = 2, | ||
| 3 | ||
| ) | ||
| EOF | ||
|
|
||
| join | ||
|
|
||
| assert_file_contents 'print(1, a = 2, 3)' | ||
| end |
There was a problem hiding this comment.
Currently this test is failing. I tried debugging it, but haven't been able to reproduce the failure. It seems to be an issue with the trailing )'s indentation, which my setup indents to the same indentation level as the print line, but the test suite is indenting to the same indentation as the arguments.
I have a few settings set on my machine that might be causing issues. The default R language indentkeys does not contain 0), meaning new lines starting with ) aren't automatically reevaluated for indentation. Using the default indentkeys, I believe the expected output should be as follows, but I'm still trying to hunt down where I might have masked this setting to be able to reproduce it.
print(
1,
a = 2,
3
)I believe this is an issue with my system settings not reflecting the unit test environment and not an issue with the code. I'm happy to help dig into it more, but it would be very helpful if you wouldn't mind adjusting the expected result to reflect whatever the default indentation produces.
There was a problem hiding this comment.
The problem has to do with R indentation differences between Vim versions -- the build uses 8.0.1453, which seems to indent the closing brackets like this. I've vendored R support, so that the build has a consistent version.
|
Thanks @AndrewRadev. At this point I think I've addressed all of your comments. Please let me know if there's anything else you would like me to follow up on or if there was anything I missed. |
|
You didn't add the documentation entry I mentioned, but I wrote it up myself -- it's not a big deal. I also found a bit of a bug in the case when the "retry at the end of the line" call would fail to join, the cursor wasn't being restored -- I applied a fix. With this, I think it's fine to merge 👍 |
sj#r#SplitFuncallwhich expands R function calls into one-line-per-argument stylesj#r#JoinFuncallwhich joins R function calls into one-line stylesj#r#ReplaceMotionPreserveCursor, which walks the text to be replaced and the replacement text to try to figure out where the cursor should end up. At least for this simple case it works well to always keep your cursor on the same character that it was on before replacing the text. For more complicated rearrangements with insertions or deletions, it would probably require some extra features.I'm still learning vimscript - open to critique and pointers if there's room to improve the code.