Skip to content

Comments

Extract delete command#420

Open
jgdhs27 wants to merge 1 commit into71:masterfrom
jgdhs27:pr420
Open

Extract delete command#420
jgdhs27 wants to merge 1 commit into71:masterfrom
jgdhs27:pr420

Conversation

@jgdhs27
Copy link
Contributor

@jgdhs27 jgdhs27 commented Jan 18, 2026

Refactor: extract delete and deletion-related commands (such as change) from the insert function into a new function named deleteSelections

Context:
For #376 I wanted to modify how deletion behaves slightly. It took me a while to understand how deletion actually works in dance (basically, you're pasting an empty string into your selection) and thought that having a separate command would make things more understandable.

Testing:

yarn test

Note that all tests still pass without modifying them.

Refactor: extract delete and deletion-related commands (such as change) from the insert function into a new function named `deleteSelections`

Testing:
```
yarn test
```
@jgdhs27 jgdhs27 marked this pull request as ready for review January 18, 2026 14:31
Copy link
Owner

@71 71 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It does make sense having a dedicated delete function, rather than relying on insert.

/**
* Delete contents of selection.
*
* @keys `d` (core: normal)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will likely conflict with yank-delete defined below.

Ideally this command would be delete with @keys a-d (no yank), and d defined below.

repetitions: number,
shift?: Argument<Shift>,
text?: Argument<string>,
text: Argument<string>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since some people may be using undefined text is their own keybindings, I think it'd be best to keep the previous behavior.

}

/**
* Delete contents of selection.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's somewhat unfortunate that this leads to both delete and deleteSelections being defined and doing the same thing. Am I correct to assume you used deleteSelections to avoid a conflict with the delete keyword? If so it may be worth naming it delete_ and teaching Dance to trim the trailing _.

Comment on lines -48 to -52
* | | | `s-r` (helix: select) | `[".edit.insert"], [".modes.set.normal"]` |
* | | | `a-d` (helix: select) | `[".edit.delete"], [".modes.set.normal"]` |
* | | | `d` (helix: select) | `[".edit.yank-delete"], [".modes.set.normal"]` |
* | | | `s-p` (helix: select) | `[".edit.paste.before"], [".modes.set.normal"]` |
* | | | `p` (helix: select) | `[".edit.paste.after"], [".modes.set.normal"]` |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why delete those?

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 this pull request may close these issues.

3 participants