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/kill and global-find-by-node work incorrectly #256

Open
mrkam2 opened this issue Mar 15, 2024 · 2 comments
Open

paredit/kill and global-find-by-node work incorrectly #256

mrkam2 opened this issue Mar 15, 2024 · 2 comments

Comments

@mrkam2
Copy link

mrkam2 commented Mar 15, 2024

Version
rewrite-clj/rewrite-clj {:mvn/version "1.1.47"}

Platform
Clojure version: 1.11.1

Symptom
Two issues:

  1. When using rewrite-clj.paredit/kill incorrectly positions the zipper after performing the operation.
  2. Docstring says that it should be removing nodes to the right from the current node. But in fact it deletes nodes starting from the current node.

Reproduction

(-> (z/of-string "[1 2 3 4]")
    z/down
    (z/insert-left (n/keyword-node :wrong-pos))
    z/next
    (z/insert-left (n/keyword-node :nil-meta))
    pe/kill
    z/print)
:wrong-pos

Actual behavior

  1. Positions zipper on the :wrong-pos.
  2. End result is [:wrong-pos 1 :nil-meta ].

Expected behavior

  1. Expected to position on 2.
  2. Expected end result is [:wrong-pos 1 :nil-meta 2]

Diagnosis

  1. Incorrect positioning is caused by global-find-by-node search that compares nodes meta not taking into account that new nodes have nil meta.
  2. Either docstring or the behavior needs to be fixed to match each other.

Action
Let me know if a PR is preferred.

@mrkam2 mrkam2 changed the title global-find-by-node works incorrectly paredit/kill and global-find-by-node works incorrectly Mar 15, 2024
@mrkam2 mrkam2 changed the title paredit/kill and global-find-by-node works incorrectly paredit/kill and global-find-by-node work incorrectly Mar 16, 2024
@lread
Copy link
Collaborator

lread commented Mar 16, 2024

Thanks @mrkam2!

I can take a peek sometime soon!

@lread
Copy link
Collaborator

lread commented Apr 27, 2024

I'm starting to take a look at this one!

  1. I confirm that the kill docstring is wrong and the behavior is correct, barring the new node bug you found.
  2. I confirm that kill behavior is broken when new nodes are involved.
  3. And yes, anything that uses paredit API's internal global-find-by-node will be broken for newly added nodes.

Thanks again for reporting, @mrkam2. Folks seem to be starting to use the paredit API a bit, and you've uncovered some very interesting flaws.

@lread lread added this to rewrite-clj Jul 3, 2024
@lread lread moved this to In Progress in rewrite-clj Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

No branches or pull requests

2 participants