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

Prevent adding new whitespace when slurping forward/backward in OCaml and Latex #631

Closed
wants to merge 4 commits into from

Conversation

taquangtrung
Copy link
Contributor

@taquangtrung taquangtrung commented Jul 15, 2016

Hi,

When using the excellent slurping feature, I encountered a problem in both OCaml and Latex. That is, a new whitespace is added when it is not supposed to:

For example: given a string (foo) List.map, I want to slurp forward to obtain (foo List.map),
the current slurping forward adds a new space between List. and map as follow:

(foo) List.map  ~~> (foo List).map ~~> (foo List .map)

The unwanted space should be removed, so that the expected behaviour is:

(foo) List.map  ~~> (foo List).map ~~> (foo List.map)

In these commits, I fixed the this problem for both forward and backward slurping, in major modes for OCaml and Latex.

Please fell free to merge them into your repository, if they are necessary.

Bests,
Trung.

@Fuco1
Copy link
Owner

Fuco1 commented Jul 15, 2016

Wow, this looks amazing on the first sight!

Could I bother you to add test cases as well? You can inspire from the python mode test file.

I'll try to review this ASAP.

@taquangtrung
Copy link
Contributor Author

taquangtrung commented Jul 16, 2016

Sure! I just added a simple test cases for OCaml mode.
I don't know how to manually test it, so I'm looking forward to your automatic testing result first.

Btw, I found out that the problem of adding more unwanted space could happen in many major modes, so fixing it by patching many modes are quite redundant (in my opinion).

I believe the root cause for forward-slurping is because of the following two lines in sp-forward-slurping-sexp

(insert " ")
(setq ins-space -1)

If the first line is removed, and the second line is changed to (setq ins-space 0), then the extra whitespace will not be added.

How do you think about it?
I don't know if this modification may affect some major modes.

@Fuco1
Copy link
Owner

Fuco1 commented Jul 16, 2016

The space-adding code is there because of lisps, because something like (foo)bar has to end up as (foo bar) rather than (foobar).

Maybe if we make the number adjustable and change it in different modes, that might be helpful too.

@taquangtrung
Copy link
Contributor Author

taquangtrung commented Jul 16, 2016

Thanks for the explanation! You are right about lisps. I have this idea about checking the sexp before and after the parenthesis or the square bracket:

Given that A and B are two sexp and we want to slurp the parenthesis in A)B:

  • If either A or B are delimiter, i.e, one of the . ; , + - *, etc, then do not add new whitespace
  • If both A and B are words (containing only letters or digits), then add new whitespace in the middle of them.

How do you think about it?

@PythonNut
Copy link
Contributor

So I'm dumb, and now we have PythonNut@cf36762, which is basically just the LaTeX portion of this, but applied to more pairs.

@Fuco1 Fuco1 added this to the 1.9 milestone Sep 23, 2016
@Fuco1
Copy link
Owner

Fuco1 commented Nov 4, 2016

@PythonNut do you want to provide it as a separate patch for now? I'll probably review this behaviour and try to come up with something universal for 1.10 release, but I would accept a temporary fix for now.

@Fuco1
Copy link
Owner

Fuco1 commented Nov 4, 2016

@taquangtrung I rebased this branch on master and the tests are failing, can you check it out? https://travis-ci.org/Fuco1/smartparens/jobs/173359035

I would like to merge this before 1.9 is out :)

@PythonNut
Copy link
Contributor

@PythonNut do you want to provide it as a separate patch for now? I'll probably review this behaviour and try to come up with something universal for 1.10 release, but I would accept a temporary fix for now.

Sure. Let me make a PR?

@Fuco1
Copy link
Owner

Fuco1 commented Nov 5, 2016

I rebased and merged this, the issue was simple for me to fix on my own. Thanks for your contribution!

@Fuco1 Fuco1 closed this Nov 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants