Bugcatching for parinfer compat functions

[sooheon]

As @noctuid's parinfer compat PRs are still not stable, this issue is to keep track of bugs/compatibility goals.

1. ], } should also be bound to lispy-barf-to-point. In parinfer, these actually work like barf-to-point-square or barf-to-point-round; i.e. they are delimiter specific. Would be nice to have.
2. Opening delimiters shouldn't wrap backwards. Expressed as a test:

(should (equal (lispy-with "(foo|)" "(")
               "(foo (|))"))

Currently, it results in ((| foo)), i.e. it is too eager to wrap.

3. ] ) } should all be possible to type inside strings.

4. Backspace should pass through closing delimiters. (should (equal (lispy-with "(foo)|" (lispy-delete-backward-or-splice-or-slurp)) "(foo|)"))

[noctuid]

they are delimiter specific

I don't know if that really makes sense though as only the closest delimiter can barf to the point. For example, with parinfer if you try to "solidify" a closing paren type that is not the innermost, instead of barfing to the point, it will just do nothing. I think another possibility would be to have ] and } to navigation commands or just leave them unbound for simplicity.

Opening delimiters shouldn't wrap backwards.

I did this on purpose. The previous wrapping behavior was that wrapping would occur if the point was at some symbol (even at the end). I actually like this behavior and think it should probably be kept the same at list in lispy-pair for consistency with all the other arguments that wrap. Maybe the the command bound to ( in the parinfer theme alone could be altered so that it would move forward a space or insert a space when at the end of a symbol.

] ) } should all be possible to type inside strings.

Yes, that should be fixed.

Backspace should pass through closing delimiters.

You mean only if they are the final delimiters? I guess parinfer technically does this, and it definitely shouldn't try to slurp up the rest of the file if you accidentally hit backspace on the final delimiter. I'll fix this as well.

I'll add one of my own:

• lispy-dedent-adjust-parens should keep the point at the beginning of a symbol.

I don't know if this should be changed in the dedent function. lispy-move-right will move the symbol to the end of the point. For example, this fails with the point being after b:

(should (string= (lispy-with "(a\n |b)" (lispy-move-right 1))
                 "(a)\n|b"))

@abo-abo Is this the intentional behavior of lispy-move-right

[sooheon]

Re: how the closing delimiters should act I disagree with leaving them unbound. Unbound just means self-insert-command, which means unbalanced parens. I'm also iffy about having ) barf to point and ]} be movement keys, just for clarity/muscle memory's sake. I think the easy default option is binding all three to barf-to-point.

Re: the wrapping backwards with ( I agree that it's useful while inside a symbol, and at the end of a symbol it should add a space and open behind the symbol.

You mean only if they are the final delimiters?

Yep, the final delimiter, or stacked against the final delimiter. Backspace should move right through something like: (foo (bar (baz)))|

I also have more issues:

• [x] There seems to be issues with stacked delimiters, either at the front or back.

(|(let (foo bar))) and backspace should give |(let (foo bar)), but it gives (let |(foo bar)).

Similarly, ((let (foo bar|))) and ) should give (let (foo bar)|)), but gives |((let (foo bar))). In this case, if there is a line above, the current line gets joined to that one as well. Annoying!

• [ ] C-d should be bound to the equivalent of lispy-delete-backward-or-splice-or-slurp, to prevent unbalancing parens.

[noctuid]

Unbound just means self-insert-command, which means unbalanced parens.

I probably should have said binding them to nil to do nothing. I think you're right; they might as well all be bound to lispy-barf-to-point. I can make a pull request for this and the self insertion in strings.

I agree that it's useful while inside a symbol, and at the end of a symbol it should add a space and open behind the symbol.

Consider this situation:

(a b c|)

If I explicitly use any of the wrapping arguments for lispy-parens (e.g. C-u (), I'd expect the c to be wrapped instead of inserting (). You're right that for the autowrapping commands it may make more sense to insert a (), but what I meant is that this should be changed in lispy-parens-auto-wrap and not in lispy-pair.

Similarly, ((let (foo bar|))) and ) should give (let (foo bar)|))

I went ahead and made a pull request for this.

[noctuid]

For reference, this what is left:

• Don't wrap backwards at the end of a symbol for autowrap commands
• Don't move from the start to the end of a symbol with the dedent command

C-d should be bound to the equivalent of lispy-delete-backward-or-splice-or-slurp, to prevent unbalancing parens.

So would this splice before an opening paren and slurp before a closing one? Would it move forward if it couldn't slurp?

Did I miss anything else?

[sooheon]

So would this splice before an opening paren and slurp before a closing one? Would it move forward if it couldn't slurp?

Yep, that's what I imagined. I just tried it out in parinfer, and there it doesn't move forward when before stacked closing parens ((foo (bar|))), but I think that's a discrepancy and it won't hurt to address it.

[noctuid]

Okay I'll add it like that. I didn't realize parinfer actually had a delete forward command. What is it bound to? C-d deletes the line in the demo.

[sooheon]

Afaik, parinfer does not have commands per se, as it just looks at state of pairs after any change and resolves them according to its rules. So actions are black boxes to it, all it cares about is reconciling pairs given certain states.

C-d deletes forwards in demo for me, as well as fn-backspace (on a mac).

[sooheon]

lispy-indent-adjust-parens and lispy-dedent-adjust-parens work differently when there is an empty line between the current sexp and the previous top-level (parent) sexp. Indenting preserves the empty line, but dedenting does not.

Test:

(should (string= (lispy-with "(foo (bar\n\n      |baz))" (lispy-dedent-adjust-parens 1))
                 "(foo (bar)\n\n     |baz)"))
(should (string= (lispy-with "(foo (bar\n\n      |baz))" (lispy-dedent-adjust-parens 2))
                 "(foo (bar))\n\n|baz"))

These tests return Unexpected rather than failing--@abo-abo can you see why?

Edit: More issues

• [x] DEL from between stacked opening parens (splicing) slurps unnecessarily

(foo (|(bar) baz)) backspace should result in: (foo |(bar) baz), instead results in (foo |(bar baz))

• [ ] Can lispy-delete-backward-or-splice-or-slurp clean up extra spaces?

The smartparens equivalent, sp-splice-sexp-killing-backward, will clean up extra space here:

((foo (| (bar) baz))

[noctuid]

Yeah that's what I thought. I just assumed it was part of the demo since C-d doesn't normally delete lines in text boxes, but you're right.

Yeah I was getting Unexpected when writing some more tests for the dedent command earlier. I'll see if I can figure out why that is.

[noctuid]

Okay, it's giving unexpected because when there is a symbol and not a list at the point, lispy-mark-list doesn't mark anything, so when lispy-different is called, it gives Unexpected. This is an error on my part. It just needs to be replaced with (set-mark (point)) which also fixes initial issue I brought up about the dedent command.

[noctuid]

Indenting preserves the empty line

Is there some reason you would want to preserve a blank line? Would it be better to also get rid of the blank line in the indent command?

[sooheon]

People usually prefer to separate their top-level sexps by empty lines, so it's nice to preserve this on a round trip of indent-dedent. Even inside of a sexp, people like separating forms with empty lines (long cond forms, for example). Also parinfer has it that way too :p

[noctuid]

Good points. adjust-parens.el also does it this way.

[sooheon]

@noctuid can you see if you can reproduce this issue? With semantic-mode on in an elisp buffer, call special-lispy-goto once (g). Quit that, and try indenting from any lispy-left position. I'm seeing semantic-analyze-possible-completions: Nothing to complete in the Messages buffer. It looks like semantic is trying to complete symbol at point when I press TAB, even when TAB is definitely bound to lispy-indent-adjust-parens.

[noctuid]

I actually can't reproduce that, but I probably have a different TAB setup than you (with yasnippet and whatnot). I don't know if that matters.

[abo-abo]

These tests return Unexpected rather than failing--@abo-abo can you see why?

Likely an error thrown by one of lispy's own functions. Try to step into that test and see what goes wrong. I typically do it like this:

1. Open a temporary buffer in emacs-lisp-mode in Window-1 and insert the text of the first arg of lispy-with there.
2. Then open the relevant functions of the second arg of lispy-with in Window-2.
3. Finally, just step through evaluations with p and xj until I see what goes wrong.

[sooheon]

@abo-abo Thanks for the pointer. I still can't picture exactly how you're using p and xj--maybe a topic for a future screencast when you find the time?

Additional bugs: @noctuid let me know if this is getting too unwieldy and you'd like separate issues.

• [ ] indent and dedent handling multiple symbols not wrapped in a sexp

From here:

(foo (bar))
|baz bif

indenting in parinfer gives:

(foo (bar)
     baz bif)

while lispy-indent-adjust-parens gives:

(foo (bar)
     baz) bif

• [ ] lispy-delete-backward-or-splice-or-slurp should not delete entire strings, imo. It should either handle doublequotes the same as other delimiters, or be ignorant and just delete them one by one, which parinfer does. I'd vote for the former. Deleting the entire string as one object, though, is different from all other pair deletion behavior, and is jarring.
• [ ] lispy-parens-auto-wrap has different behavior from [ or { inside strings. [ and { also insert the closing pair, ( doesn't.

[noctuid]

indent and dedent handling multiple symbols not wrapped in a sexp

Thanks, I hadn't noticed that.

lispy-delete-backward-or-splice-or-slurp should not delete entire strings

I'm not sure about this one. I don't think it's a good idea to follow parinfer and just unbalance the string by deleting one quote. I don't think it's really useful to splice a string or slurp something into it either, and that would require adding string splicing and slurping commands.

lispy-parens-auto-wrap has different behavior from [ or { inside strings. [ and { also insert the closing pair, ( doesn't.

lispy-parens does this as well though, so this isn't caused by the parinfer version. I'm not sure if this is deliberate.

[noctuid]

DEL from between stacked opening parens (splicing) slurps unnecessarily

I can't reproduce this. Do you still have this problem with the most recent commit?

The smartparens equivalent, sp-splice-sexp-killing-backward, will clean up extra space here:

I'll look at this.

let me know if this is getting too unwieldy and you'd like separate issues.

It should be okay. Please make new comments instead of edits though; I only just now noticed the edit.

[sooheon]

DEL from between stacked opening parens (splicing) slurps unnecessarily

Has been fixed.

Re doublequote deletion, on second thought, I agree with you. One rarely needs to splice or slurp strings. What do you think about this behavior though: what I really wanted was for backspace to pass through the closing doublequote (leaving it in place). Deleting the opening double quote could delete the entire string.

[noctuid]

Okay the problem with the indent function where it would work incorrectly with multiple sexps on the same line is fixed now.

I was thinking that it might make sense to skip over the quotes and only delete an empty string. I think your suggestion of deleting the whole string at the front quote is probably more consistent with splicing and more useful, so I'll have it do that.

[sooheon]

That sounds good. I've found a couple more bugs:

In a completely empty buffer, lispy-parens-auto-wrap errors with "Wrong type argument: number-or-marker-p, nil"

All of the auto-wrap functions wrap incorrectly in the following position: ("string"|) resulting in [| ("string")]

[sooheon]

Also, I have an idea about differentiating ) ] and }. When typing code such as this:

(let [print-number (fn [n]
                     (when (> n 0)
                       (print-number (dec n|))))])

Imagine you've just finished typing the dec n form. Wouldn't it be great if ] took you to the end of the let binding immediately? So each delimiter would look to barf the nearest occurrence of that closing delim, and if that's impossible, it would jump to the nearest occurrence instead. You would lose the ability to barf-to-point any delimiter with any one of )]}, but it's not too much to lose, when you consider 1. when you type normally, you close with the actual delimiter you want anyway, and 2. you can gain a lot in specific navigation for free.

[noctuid]

when you type normally, you close with the actual delimiter you want anyway,

Yes and that's also how it is for parinfer. I think your suggestion makes sense.

@abo-abo lispy--bounds-dwim will return return the bounds of symbol in this situation:

(a symbol|)

However in this situation it will return the bounds of the list:

(a "string"|)

Is this the intended behavior?

[abo-abo]

Is this the intended behavior?

Not really, it's a late stage fall-through: (ignore-errors (bounds-of-thing-at-point 'sentence)). I updated it just now.

[noctuid]

@abo-abo Thanks. Also, @sooheon pointed out that the dedent command should keep blank lines. Do you think this should also apply to lispy-move-right or should it continue to get rid of blank lines. For example,

(should (string= (lispy-with "(a\n\n ~b|)" (lispy-move-right 1))
                 "(a)\n\n~b|"))

[abo-abo]

I like getting rid of blank lines. If the user really wants them, they should be marked with a region - they're not deleted in that case.

[noctuid]

@abo-abo Okay. I also had a question about lispy-parens. Is lispy-parens supposed to insert both the opening and closing paren in strings? It doesn't for me, but if I eval (lispy-with "\"|\"" (lispy-parens nil)), I get "\"(|)\"".

If I try running make test with a test that has something like (lispy-with "\"|\"" "("), I get

Test lispy-parens condition:
    (args-out-of-range
     []
     0)

[abo-abo]

Works OK for me:

(lispy-with "\"|\"" "(")
;; => "\"(|)\""

[noctuid]

@abo-abo I realize why it was inserting both parens with the actual function call (instead of "(") from looking at lispy-pair. I don't understand why I'm getting an error when trying (lispy-with "\"|\"" "("), but shouldn't it only insert one paren because of this?

((lispy--in-string-or-comment-p)
 (if (and (string= ,left "(")
          (= ?\( (aref (this-command-keys-vector) 0))) ; < here
     (insert "(")
   (insert ,left ,right)
   (backward-char 1)))

I'm not sure I understand this. Is there some reason for having lispy-parens behave differently when bound to ( vs. another character?

Also, the documentation says it should only insert the opening paren in a string:

(   "a string ("
)   "a string )"
{   "a string {}"
}   "a string []"

EDIT: Also, could lispy--in-string-p and lispy--in-string-or-comment-p be made consistent? For example, here the former will return the point while the latter will return nil:

|"a string"

I don't know if this would break something. If kept the same, it should probably be mentioned in the docstrings.

[noctuid]

@sooheon I added the commands you suggested, but I didn't bind them by default. I'm not sure if it makes sense to since } would pretty much only be useful for clojure and ] for clojure and sometimes scheme.

[sooheon]

Nice, thanks. I guess it's going to soon be a matter of documentation for all these new features.

[sooheon]

One more issue:

lispy-delete-backward-or-splice-or-slurp is too eager when deleting a non-top-level empty pair on a newline.

(foo
 (|))

del should give:

(foo
 |)

But it leaves

(foo|)

Contrast with lispy-delete-backward, or deleting from:

(foo
 (|)
 bar)

[sooheon]

The auto-wrap opening delimiters should not add a space when called immediately after # in clojure.

Also, the auto-wrap delimiters bug out when called immediately after ^.

[noctuid]

The auto-wrap opening delimiters should not add a space when called immediately after # in clojure.

Same for other lisps. I'll fix this if/when #271 is merged.

Also, the auto-wrap delimiters bug out when called immediately after ^.

I can't replicate this. Is this still a problem?

Apart from those, I think the only other remaining issue is the dedent command getting rid of newlines. Have you noticed any other problems?

[sooheon]

Also, the auto-wrap delimiters bug out when called immediately after ^.

This problem is gone for me as well.

Regarding the auto-wrap functions, they still behave wonky within strings. They should not add closing pairs, and they should not add preceding spaces.

Also, I'm finding that requiring a negative arg for auto-wrap functions to never wrap is a bit annoying.

Currently:

1. no args: wrap to eol or sexp
2. universal arg: same as above
3. negative arg: don't wrap
4. 0 arg: wrap maximum
5. pos arg: wrap that many sexps

What do you think about 0 arg not wrapping, and the universal arg wrapping to maximum?

[noctuid]

they should not add preceding spaces

I think I inadvertently fixed this in the pull request I just made. I should probably add tests and an explicit check though.

They should not add closing pairs

The auto-wrap commands behave the same as the normal commands here; I've never touched the code for lispy-pair in a string or comment. I prefer that they add closing pairs personally and think lispy-pair should handle ( differently, but that's an unrelated issue.

universal arg: same as above

This is actually the same as an arg of 1.

What do you think about 0 arg not wrapping, and the universal arg wrapping to maximum?

I'm not sure. One the one hand, I'd like to keep the arguments basically the same. On the other hand, you're right that it might be nicer to have not wrapping as 0. Just swapping no arg and -1 is the least surprising thing to do, so I think it should probably be kept like this by default. You could always use a small wrapper to change the arg yourself.