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

Add evil-cleverparens layer #2747

Closed
wants to merge 1 commit into from
Closed

Add evil-cleverparens layer #2747

wants to merge 1 commit into from

Conversation

justbur
Copy link
Contributor

@justbur justbur commented Aug 24, 2015

Seems like a nice package. See https://github.com/expez/evil-smartparens

Thought I'd add it as a layer before possibly adding it to the spacemacs layer.

@robbyoconnor
Copy link
Contributor

👍

@robbyoconnor
Copy link
Contributor

Though I think it belongs in the spacemacs layer personally

@cmccloud
Copy link
Contributor

With sp strict mode enabled, D is deleting more than a single sexp here, are you seeing the same behavior?

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

Can you give an example?
On Tue, Aug 25, 2015 at 2:16 PM Christopher McCloud <
notifications@github.com> wrote:

With sp strict mode enabled, D is deleting more than a single sexp here,
are you seeing the same behavior?


Reply to this email directly or view it on GitHub
#2747 (comment).

@cmccloud
Copy link
Contributor

With evil, smart-parens, smart-parens-strict, and evil-smartparens enabled in an emacs-lisp buffer

(defun test-sexp ()
   |(sexp-A
     (A-first-child))
    (sexp-B)
    (sexp-C
     (C-first-child)
     (C-second-child)))

My expectation is that "D" should delete sexp-A and it's child, but leave sexp-B and sexp-C, instead D results in:

(defun test-sexp ()
    |)

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

I see the same thing, and I would have the same expectation. Maybe you can
open an issue?

On Tue, Aug 25, 2015 at 2:25 PM, Christopher McCloud <
notifications@github.com> wrote:

With evil, smart-parens, smart-parens-strict, and evil-smartparens enabled
in an emacs-lisp buffer

(defun test-sexp ()
|(sexp-A
(A-first-child))
(sexp-B)
(sexp-C
(C-first-child)
(C-second-child)))

My expectation is that "D" should delete sexp-A and it's child, but leave
sexp-B and sexp-C, instead D results in:

(defun test-sexp ()
|)


Reply to this email directly or view it on GitHub
#2747 (comment).

@cmccloud
Copy link
Contributor

I want to see if it's a spacemacs thing before I do that, but if I replicate this consistently then yes, I will.

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

It's not. Here is the problematic function: https://github.com/expez/evil-smartparens/blob/master/evil-smartparens.el#L86

@cmccloud
Copy link
Contributor

Thanks @justbur, nice catch.

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

I don't know much about this package. It just seemed like a good idea, and
besides this issue it has not surprised me too much yet.

On Tue, Aug 25, 2015 at 2:41 PM, Christopher McCloud <
notifications@github.com> wrote:

Thanks @justbur https://github.com/justbur, nice catch.


Reply to this email directly or view it on GitHub
#2747 (comment).

@Kethku
Copy link
Contributor

Kethku commented Aug 25, 2015

I also wonder if we should take a look at lispy, which I believe has a fair amount of overlap to this package. I have been meaning to write a configuration layer for it for a while, but just haven't found the time. It does bring up the question of which of the many lisp editing packages should be included in spacemacs. I believe there is even some overlap already implemented for editing sexps. Just something to think about.

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

I don't see anything wrong with having a layer, like lispy, that replaces
"built-in" (spacemacs layer) functionality. There's a way to exclude
packages within a layer I believe.

This one works joins evil and smartparens which are both included by
default.

On Tue, Aug 25, 2015 at 3:06 PM, Devagamster notifications@github.com
wrote:

I also wonder if we should not take a look at lispy, which I believe has a
fair amount of overlap to this package. I have been meaning to write a
configuration layer for it for a while, but just haven't found the time. It
does bring up the question of which of the many lisp editing packages
should be included in spacemacs. I believe there is even some overlap
already implemented for editing sexps. Just something to think about.


Reply to this email directly or view it on GitHub
#2747 (comment).

@Kethku
Copy link
Contributor

Kethku commented Aug 25, 2015

I suppose having more options is good :)

@cmccloud
Copy link
Contributor

@Devagamster for what it's worth i added a really basic layer for lispy that you might use as a starting point. I've basically added lispy conditionally to evil-emacs mode since there are many buffers which use evil-emacs where lispy is inappropriate.

Initially I thought to model a lispy layer after the built in lisp-state, but after using it more I didn't think it warranted an additional state.

https://github.com/cmccloud/.spacemacs.d/blob/master/layers/lispy/packages.el

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

Interesting comment on the bug report. @cmccloud what do you think of https://github.com/luxbock/evil-cleverparens? I wasn't aware of it

@cmccloud
Copy link
Contributor

I'll take a look, it actually sounds incredible (as did/does evil-smartparens).

If the thought is that one of these packages might end up in spacemacs by default, I do think it makes more sense to pick the one that comforms as closely as possible to sp or evil behavior. Even if a greedy "D" is useful, I don't want to add more for new users (and new lispers) to have to keep track of.

Edit: Along those same lines, I think it would be good to hear from spacemacs users new to lisp generally before setting any kind of structured editing mode as a default.

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

I agree with you that the default behavior for "D" is unusual. Why don't
you give cleverparens a shot? I don't have time to play with it much right
now.

On Tue, Aug 25, 2015 at 3:31 PM, Christopher McCloud <
notifications@github.com> wrote:

I'll take a look, it actually sounds incredible (as did/does
evil-smartparens).

If the thought is that one of these packages might end up in spacemacs by
default, I do think it makes more sense to pick the one that comforms as
closely as possible to sp or evil behavior. Even if a greedy "D" is useful,
I don't want to add more for new users (and new lispers) to have to keep
track of.


Reply to this email directly or view it on GitHub
#2747 (comment).

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

@cmccloud I don't like the behavior of "D" in your example with cleverparens either.

@cmccloud
Copy link
Contributor

@justbur you think it should clean-up afterwards?

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

I liked your suggestion for the case you pointed out, but at least the
cleverparens behavior seems easy to understand.

On Tue, Aug 25, 2015 at 4:34 PM, Christopher McCloud <
notifications@github.com> wrote:

@justbur https://github.com/justbur you think it should clean-up
afterwards?


Reply to this email directly or view it on GitHub
#2747 (comment).

@cmccloud
Copy link
Contributor

sorry, what behavior are you seeing with cleverparens, using my example from earlier?

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

Oh that it just deletes the line leaving an unbalanced expression.
On Tue, Aug 25, 2015 at 4:42 PM Christopher McCloud <
notifications@github.com> wrote:

sorry, what behavior are you seeing with cleverparens, using my example
from earlier?


Reply to this email directly or view it on GitHub
#2747 (comment).

@cmccloud
Copy link
Contributor

I see exactly what I expected from my earlier example - identical to what calling sp-kill-sexp produces.

@justbur
Copy link
Contributor Author

justbur commented Aug 25, 2015

That's weird. It worked for me the second time.

@justbur
Copy link
Contributor Author

justbur commented Aug 26, 2015

@cmccloud Switched the pr to cleverparens. I like the movement commands it adds too

@justbur justbur changed the title Add evil-smartparens layer Add evil-cleverparens layer Aug 26, 2015
@Kethku
Copy link
Contributor

Kethku commented Aug 26, 2015

you squashed the commits and everything. good man

@Kethku
Copy link
Contributor

Kethku commented Aug 26, 2015

or women...

@cmccloud
Copy link
Contributor

@justbur I really like cleverparens too - I've encountered a few bugs, but overall I think it's great.
I made an upstream pr for one issue here emacs-evil/evil-cleverparens#8
It also seems like dd can sometimes produce weird results when dealing with quoted lists, just as a heads up if you notice it and want to try and track it down.

@sooheon
Copy link

sooheon commented Aug 27, 2015

I've got an opened issue for evil-cleverparens that hasn't had a response in a while. C will put the cursor a line below, not something I'd want to deal with by default.

@sooheon
Copy link

sooheon commented Aug 27, 2015

@Devagamster and @cmccloud I'm also using lispy (and other abo-abo packages) extensively, and I've been overriding a lot of spacemacs with it. I'd be happy to contribute to a layer as well.

@justbur
Copy link
Contributor Author

justbur commented Aug 27, 2015

I really like the idea of some minimal changes to evil to make it respect
balanced sexps, especially with smartparens in strict mode. If we have to
we can find the solution that's closest to what we want, fork it and make
the necessary changes. It shouldn't be too hard.

On Thu, Aug 27, 2015 at 11:26 AM, Sooheon Kim notifications@github.com
wrote:

I've got an opened issue for evil-cleverparens that hasn't had a response
in a while. C will put the cursor a line below, not something you want.
Not something I'd want to deal with by default.


Reply to this email directly or view it on GitHub
#2747 (comment).

@sooheon
Copy link

sooheon commented Aug 27, 2015

I agree, it's very much in the spirit of spacemacs. Just wish the package was maintained, but I also agree we could maintain a fork as well.

@justbur
Copy link
Contributor Author

justbur commented Aug 27, 2015

@sooheon Do you have an opinion on evil-smartparens? That's what we started with here

@justbur
Copy link
Contributor Author

justbur commented Aug 27, 2015

@cmccloud I think the function you wanted for your original example is sp-kill-hybrid-sexp. It's easy enough to bind that to D

@Kethku
Copy link
Contributor

Kethku commented Aug 27, 2015

@sooheon, I think adding lispy should happen similarly to this: #1417. Give an option for the more opinionated layer but leave the default with something like evil/clever-parens (maybe a mix? take the stuff that works from each)

@sooheon
Copy link

sooheon commented Aug 27, 2015

@justbur For than the D Y C behaviour with sexp with newlines mentioned above, dd, yy, cc also do not work in the same case. There's also dk, dj which have tons of little edge cases. These are just annoyances, I can't remember why I actually abandoned it, I think I came across a slightly worse bug (or interaction w/ some other package) but can't remember.

@cmccloud
Copy link
Contributor

@sooheon If you're willing to share what you've got so far, I would like to take a look at what you've put together so far with lispy. Maybe we can open a new PR for a lispy layer and whoever is interested can start to contribute.

What I really like about evil-cleverparens is that it's robust enough to work as someones primary structured editing tool, and at the same time the changes it introduces are subtle enough that it doesn't get in the way otherwise. I could imagine it being enabled by default without upsetting anyone.

@sooheon @luxbock has made some contributions to spacemacs, so maybe we can get a hold of them and see if they would be willing to make a few changes for us.

@justbur
Copy link
Contributor Author

justbur commented Aug 27, 2015

Thanks @sooheon. The author of evil-smartparens said something like he
doesn't want to replicate smartparens functions, but I'm thinking
that's exactly what I want here. I just want sensible replacements for
D, Y, C, etc that don't leave unbalanced expressions, and I think
using the smartparens functions as much as possible is the safest way
to achieve that.

In the D example above for instance, maybe we should just make a layer
that rebinds D to sp-kill-hybrid-sexp, and fills out the rest of the
keys using smartparens where possible and writes some simple functions
for the rest. How does that sound?

On 8/27/15, Sooheon Kim notifications@github.com wrote:

@justbur For than the D Y C behaviour with sexp with newlines mentioned
above, dd, yy, cc also do not work in the same case. There's also dk, dj
which have tons of little edge cases. These are just annoyances, I can't
remember why I actually abandoned it, I think I came across a slightly worse
bug (or interaction w/ some other package) but can't remember.


Reply to this email directly or view it on GitHub:
#2747 (comment)

@sooheon
Copy link

sooheon commented Aug 27, 2015

I think that would at least be very clear what its trying to be, and useful. It should probably have a different name though. parevil? evil-parens?

@justbur
Copy link
Contributor Author

justbur commented Aug 27, 2015

yes, different than any existing package. Whatever you guys what is fine
for the name.

I'll work on updating the pr. Just propose commands as you find them, and I
can add them. I'll start with D mapped to sp-kill-hybrid-sexp. What if dd
just went to the beginning of the line and then did sp-kill-hybrid-sexp?
That's roughly what I would expect it to do.

On Thu, Aug 27, 2015 at 1:11 PM, Sooheon Kim notifications@github.com
wrote:

I think that would at least be very clear what its trying to be, and
useful. It should probably have a different name though. parevil?
evil-parens?


Reply to this email directly or view it on GitHub
#2747 (comment).

@luxbock
Copy link

luxbock commented Aug 28, 2015

I'd he happy to see evil-cleverparens introduced as a layer to Spacemacs, and I'm definitely open to feedback and changes if you guys have any ideas on what direction to take the package. The package was born out of a hacking spree that unfortunately ran out of steam before I got all of the issues sorted out, but I'll try to fix the most obvious ones today. If you have any other suggestions / bug reports please file them as issues in its repo and I'll try my best to be more responsive than I have been.

EDIT: I think I've fixed the behavior of S, C and cc to work as I originally intended.

@justbur for ideal evil integration just replacing the binding of a key in normal-state isn't enough, as then you lose out on features like custom yank-handlers. I find it really handy to be able to use "_ to not have the delete commands insert into the kill-ring for example. This is why the delete/change code in evil-cleverparens is as long as it is, as I've chosen to inline code from other packages to achieve this behavior.

@justbur
Copy link
Contributor Author

justbur commented Aug 28, 2015

@luxbock I realized quickly yesterday that I would need to understand evil operators better to do this, so I'm happy to stick with cleverparens if you're willing to stay on top of any issues that come up.

@luxbock
Copy link

luxbock commented Aug 28, 2015

The biggest issue right now are quoted lists and unbalanced delimiters inside comments/strings which sometimes cause issues. I remember taking a look at this earlier without being able to come up with a fix but I'll give it another try today.

There are some other bugs as well which I'm aware of (with > leaving trailing whitespace for example) but haven't filed as issues on the repo, but those are rather minor and should be rather easy to fix.

@luxbock
Copy link

luxbock commented Sep 7, 2015

I believe most of the issues raised here should now be fixed, and I've also added a few more features as well! I still have to update the README as it's quite outdated at this point.

@PierreR
Copy link
Contributor

PierreR commented Oct 17, 2015

I believe the default smartparens is poorly integrated in spacemacs.

If this PR is meant to fix this, then 👍

@StreakyCobra
Copy link
Contributor

@justbur You have to rebase your README on top of the template, TheBB did some changes for the online documentation.

@robbyoconnor
Copy link
Contributor

@justbur You have to rebase your README on top of the template, TheBB did some changes for the online documentation.

👍

@justbur
Copy link
Contributor Author

justbur commented Nov 12, 2015

@StreakyCobra @robbyoconnor done

@syl20bnr
Copy link
Owner

Thank you 👍
I will use it for some time and if it works correctly I may move it to spacemacs distribution since it is a toggle.
Cherry-picked into develop branch, you can safely delete your branch.

@syl20bnr syl20bnr closed this Nov 27, 2015
@luxbock
Copy link

luxbock commented Nov 27, 2015

@syl20bnr I've tried to make all features that some might consider intrusive to be optional, but right now some of the defaults might be surprising to some who expects the bare minimum. For example there is a special version of evil-insert that inserts an extra space before the cursor when the point is between an opening parentheses and a symbol, which is a feature I find really helpful (for mostly for auto-complete) but may come as a surprise to someone not expecting it. I turned on many of these features on by default since I figured most people would never give them a try otherwise, but if you are thinking about including the package in Spacemacs itself then it might be better to configure some of those defaults to be more conservative.

Like I said earlier I'm quite open to making changes here and there if the community feels strongly about any aspect of the package, so feel free to drop issues and concerns my way.

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.

9 participants