bhurlow / vim-parinfer

vim plugin to balance your parenthesis
MIT License
182 stars 18 forks source link

Double line breaks should not split expressions #27

Closed js-choi closed 7 years ago

js-choi commented 7 years ago

Vim-Parinfer currently correctly considers this text as one form:

(a b
 c
 d)

Editing this text with Vim-Parinfer (e.g., by exiting insert mode) correctly makes no changes.

However, Vim-Parinfer currently incorrectly considers this as two forms:

(a b
 c

 d)

Editing this text with Vim-Parinfer will incorrectly change this text. If its first two lines are edited, Vim-Parinfer will change the text to:

(a b
 c)

 d

…and if the text’s last line is edited, Vim-Parinfer will change it to:

(a b
 c

 (d)

This is apparently due to how Vim-Parinfer currently determines what text to send to the Parinfer.js Node process. It seems to incorrectly assume that double line breaks always split expressions—sending only text blocks delimited by double line breaks, rather than seeking the actual boundaries of the outermost expression.

bhurlow commented 7 years ago

@js-choi yep, current impl separates forms by spaces. This is not ideal as lots of people like to separate internal forms with spaces. Vim-sexp looks for the first paren of the form by looking for ( on column1, which is a nice solution since it's quite high(er) performance than searchpairpos

bhurlow commented 7 years ago

I have this done elsewhere and can add. Soon though, the parinfer code will be replaced with the raw VimL version over the js rpc

js-choi commented 7 years ago

Are there still plans to add this in soon? Replacing the Node implementation with VimL will be nice, but if the fix for the Node version is already done, then it’d be really useful as a stopgap.

But this is a volunteer project, and I can’t help but appreciate what you’ve already provided. Thanks again.

bhurlow commented 7 years ago

@js-choi ok cool thanks for the bump. I'll try to get going on the Viml version this week

tomaskulich commented 7 years ago

+1, this is great project and would be even better if this issue was fixed!

bhurlow commented 7 years ago

Ok, pulling in the viml library (no more js rpc) and tweaking the top level form search addresses this in most cases: https://github.com/bhurlow/vim-parinfer/commit/3a1202620558726d0fe068c3836e1825e99835c1

tomaskulich commented 7 years ago

The behavior of the plugin is too buggy for me. I created an infinitely simplified version of it in case anyone is interested: https://github.com/tomaskulich/vim-parinfer-bare

bhurlow commented 7 years ago

@tomaskulich yea, gotta agree with you there :). What did you do for Select_full_form? Happy to merge these together if you're liking your approach more

bhurlow commented 7 years ago

@tomaskulich, looks like you're just selecting the full file... Does this work ok for you?

bhurlow commented 7 years ago

I was hoping to only send the minimal amount of data to Parinfer. The sticky part here is how to select the full form if there are missing parens. If there are missing parens searchpairpos() wont find the correct section to eval. Previously I was using empty lines to determine this but that precludes line separations within forms

js-choi commented 7 years ago

Closing this issue because the double-line-break issue has been fixed, though regressions have indeed occurred; see also #32.

bhurlow commented 7 years ago

@js-choi thanks. Can you please post the regressions you encounter?

js-choi commented 7 years ago

I will try when I get a bit of spare time; these ones seem to be harder to characterize, though.