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

paredit design flawed around automatically slurping into empty seqs #339

Closed
lread opened this issue Feb 7, 2025 · 5 comments · Fixed by #359
Closed

paredit design flawed around automatically slurping into empty seqs #339

lread opened this issue Feb 7, 2025 · 5 comments · Fixed by #359

Comments

@lread
Copy link
Collaborator

lread commented Feb 7, 2025

Background

Normally, in an editor, you slurp into the sequence in which you are located.

  • in seq (:a |:b :c) :d :e => (:a |:b :c :d) :e
  • in empty seq (|) :a :b => (|:a) :b

But...

A zipper has no concept of being located within an empty seq.
The granularity is the node. I can be located at |() but not at (|).

So...

The rewrite-clj paredit API made a design choice.
If you are located at an empty seq, you slurp into that empty seq and then are navigated to the slurped item.

|() :a :b => (|:a) :b

But...

This makes certain slurp scenarios impossible.

Given (:a |() :b) :c :d)
In an editor you'd expect a slurp result of (:a |() :b :c) :d)
But rewrite-clj paredit will, because you happen to be at an empty seq, automatically slurp into that empty seq (:a (|:b) :c) :d)

Well, just navigate one raw node back you say to compensate, you say.
That's awkward but OK:

(:a| () :b) :c :d) => (:a| () :b :c) :d)

But what if there is no space node before our loc to navigate back to, I ask?

(|() :b) :c :d) <- no way not to slurp into empty sequence currently.

Options

Option 1: Leave it

Live with the flaw.

Pros:

  • no breaking changes

Cons:

  • certain slurp scenarios won't be possible with rewrite-clj paredit.

Option 2: Introduce a slurp-forward-into

Change slurp-forward to never automatically slurp forward into empty sequences.
Introduce a new slurp-forward-into which would slurp into the seq at the current location.
This puts the control in the user's hands rather than rewrite-clj paredit automatically making a decision.

sub-option 2a
slurp-forward-into locates to the slurped item

  • |() :a :b => (|:a) :b
  • |(:a :b) :c _d => (:a :b |:c) :d

sub-option 2b
slurp-forward-into preserves location

  • |() :a :b => |(:a) :b
  • |(:a :b) :c _d => |(:a :b :c) :d

I think sub-option 2b is potentially less surprising.
The user can always decide to navigate to the slurped item after slurping forward into.

Pros:

  • Slurping into is a deliberate rather than automatic choice

Cons:

  • Breaking change to users of slurp-forward
  • I expect this will be a low impact because few folks use this API, but still.

Option 3: introduce slurp-forward options

Same idea as 2, but:

  • (slurp-forward zloc) behaves the same as today with its flawed automatic slurping into empty seqs
  • (slurp-forward zloc {:into true}) - same behaviour as slurp-foward-into in option 2.
  • (slurp-forward zloc {:into false}) - same behaviour as slurp-forward in option 2
  • (slurp-forward zloc {:into :auto}) - the default with option explicitly expressed

Pros:

  • non-breaking
  • extensible if we want/need to add other options

Cons

  • default is flawed for some use cases

Option 4...

I'm sure there are other ideas we could explore.

Choice

I'm leaning toward Option 3. Feedback is most welcome and appreciated.

Notes

  • Also applies to other rewrite-clj paredit slurp fns.
@lread
Copy link
Collaborator Author

lread commented Feb 7, 2025

Option 4: Add slurp-forward-into and deprecate slurp-forward

  • slurp-forward current behaviour preserved, but is deprecated.

  • (slurp-forward-into zloc {:from :parent}) slurp by searching upward from the current node (:a |() :c) :d => (:a |() :c :d)

  • (slurp-forward-into zloc) same as (slurp-forward-into zloc {:from :parent}) (:upward is the default)

  • (slurp-forward-into zloc {:from :current}) slurp into the current node (:a |() :c) :d) => (:a |(:c)) :d)

Pros

  • non-breaking
  • extensible if we want to add other options
  • guides user to what we feel they should be using through deprecation

Cons

  • not sure yet. must be some.

Now leaning toward option 4. Feedback is most welcome.

@acamargo
Copy link

acamargo commented Feb 10, 2025

Is there any specification regarding the expected behavior of slurp?

I prefer the non-breakable behavior of option 4.

Regarding slurp-forward-into, it feels more natural to me switching from :current to :parent when there is no rightmost node available.

Example: (:a |() :c) :d =slurp-forward=> ( :a |(:c)) :d =slurp-forward=> (:a |(:c) :d) =slurp-forward=> (:a |(:c :d)) =slurp-forward=> (:a |(:c :d)) "no effect because there are no rightmost nodes left to be slurped"

But I don't have a strong opinion on that.

I've also tested on other editors/plugins to see how it goes:

editor/scenario (:a |() :c) :d
VSCode + Calva => (:a |() :c :d) => NOP
IntelliJ + Cursive => (:a |() :c :d) => NOP
NeoVim + vim-sexp-mappings-for-regular-people => (:a ( :c|)) :d => NOP
Emacs + Doom => (:a |() :c :d) => NOP
editor/scenario (:a (|) :c) :d
VSCode + Calva => (:a (| :c)) :d => (:a (| :c) :d) => (:a (| :c :d)) => NOP
IntelliJ + Cursive => (:a (|:c)) :d => (:a (|:c) :d) => (:a (|:c :d)) => NOP
NeoVim + vim-sexp-mappings-for-regular-people => (:a ( :c|)) :d => NOP
Emacs + Doom => (:a (|:c)) :d => (:a (|:c) :d) => (:a (|:c :d)) => NOP
editor/scenario (:a ()| :c) :d
VSCode + Calva => (:a ()| :c :d) => NOP
IntelliJ + Cursive => (:a ()| :c :d) => NOP
NeoVim + vim-sexp-mappings-for-regular-people => (:a () :c :d|) => NOP
Emacs + Doom => (:a ()| :c :d) => NOP

@lread
Copy link
Collaborator Author

lread commented Feb 10, 2025

Thanks @acamargo, your feedback is much appreciated!

Is there any specification regarding the expected behavior of slurp?

Not that I know of. The original author of rewrite-clj.paredit referenced http://pub.gajendra.net/src/paredit-refcard.pdf (which I left in the namespace docstring).

Regarding slurp-forward-into, it feels more natural to me switching from :current to :parent when there is no rightmost node available.

Example: (:a |() :c) :d =slurp-forward=> ( :a |(:c)) :d =slurp-forward=> (:a |(:c) :d) =slurp-forward=> (:a |(:c :d)) =slurp-forward=> (:a |(:c :d)) "no effect because there are no rightmost nodes left to be slurped"

But I don't have a strong opinion on that.

If I've understood your comment, I think you should be happy. You can think of the :from as the magnet and ultimate destination for the slurpee, but we only slurp one step at a time. So when :from is :current:

  1. (:a |() :c) :d
  2. (:a |(:c)) :d - matching seq/slurpee candidate was () :c so we slurped :c into ()
  3. (:a |(:c) :d) - matching seq/slurpee candidate was (:a (:c)) :d so we slurped :d into (:a (:c))
  4. (:a |(:c :d)) - matching seq/slurpee candiate was (:c) :d so we slurped :d into (:c)
  5. (:a |(:c :d)) - no matching seq/slurpee candidate no-op.

Thanks for playing with various editors; very interesting.
I find NeoVim behaviour odd.
I think the Emacs+Doom behaviour is the kind of behaviour the paredit slurp behaviour should more closely match (at least from your examples above).

@acamargo
Copy link

which I left in the namespace docstring

My bad for not having read the docstring. Thanks for sharing the reference! 🙇

If I've understood your comment, I think you should be happy.

I am happy indeed; :current seems to perform exactly as I expected. 😄

I think the Emacs+Doom behaviour is the kind of behaviour the paredit slurp behaviour should more closely match

Agreed. Although I'm not an Emacs user, I'd say it sets the standard due to historical reasons.

What's the next step?

@lread
Copy link
Collaborator Author

lread commented Feb 10, 2025

What's the next step?

I continue to review and fix the paredit API. Getting there! It needed lots of love.

lread added a commit that referenced this issue Feb 18, 2025
Because of #256, slurp fns had to be rewritten. During rewriting:

- address design flaw by deprecating existing `slurp` fns and adding
replacement `slurp-`*`-into` fns (closes #339)
- stop adding space char when preserving slurped newlines (closes #345)
- review ambiguous `-slurp-`*-`fully` fn behaviour (closes #341)
- when slurping, don't consider a node with `#_` uneval nodes
empty (closes #338)
- don't throw on rewrite-clj parseable but invalid clojure
`{:a}` (closes #336)
- slurping forward fully no longer throws when slurping into an empty
seq that is last item in a seq (closes #335)
- slurping backward at empty-seq at start of seq no longer
throws (closes #334)
- slurp forward now slurps when at empty-seq at end of seq (closes #333)
@lread lread closed this as completed in 3954f58 Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants