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

Use same function arguments for sp-kill-region as kill-region #1047

Closed
wants to merge 1 commit into from

Conversation

dakra
Copy link

@dakra dakra commented Sep 17, 2020

Some packages like whole-line-or-region expect the same
function signature for kill-region kind of functions.

Now you can e.g. define

  (defun whole-line-or-region-sp-kill-region (prefix)
    "Call `sp-kill-region' on region or PREFIX whole lines."
    (interactive "p")
    (whole-line-or-region-wrap-region-kill 'sp-kill-region prefix))

and call whole-line-or-region-sp-kill-region (probably mapped to C-w)
to remove the whole line but only if it keeps the balance.

@Fuco1
Copy link
Owner

Fuco1 commented Oct 8, 2020

I don't know if this is necessary. The package also does wrap delete-region with function whole-line-or-region-wrap-beg-end so I think you could simply use that instead of the ...wrap-kill-region version.

There seems to be no point to the third argument in whole-line-or-region-wrap-region-kill anyhow, since it passes the region boundaries to the function as aguments so the third argument seems to be just a compat layer.

If we can make this work I'd rather not add more complexity.

@dakra
Copy link
Author

dakra commented Oct 12, 2020

EDIT: It only worked because I still had the patched sp-kill-region version that takes 3 arguments.
whole-line-or-region-wrap-beg-end also calls the function with 3 arguments.
It works with delete-region because (surprisingly) you can call delete-region with 3 or more arguments
and it still works.

-- cut --
Here my old message what I wrote before I realized I still use the patched sp-kill-region:

Hmm, ok. I just tried it with

(defun whole-line-or-region-sp-kill-region (prefix)
  "Call `sp-kill-region' on region or PREFIX whole lines."
  (interactive "p")
  (whole-line-or-region-wrap-beg-end 'sp-kill-region prefix))

and this works just as well.
I'm a bit confused why then there even is a different
..-region-kill function.
It's the same but it calls 2 macros first,
whole-line-or-region-filter-with-yank-handler, and
whole-line-or-region-preserve-column.
Which docstrings say "Ensure the current column is kept the same after executing BODY."
and "[...] The binding ensure killed strings have a yank handler attached".

I tried experimenting a bit and using those 2 functions in various
ways (killing and yanking multiple lines etc) but I couldn't find a
difference.

So I think you can close this PR but I'm still curious what I'm
missing and what those macros actually do.

Thanks for your time and this great package.
(PS you should add Github sponsors)

Some packages like `whole-line-or-region` expect the same
function signature for `kill-region` kind of functions.

Now you can e.g. define

```
  (defun whole-line-or-region-sp-kill-region (prefix)
    "Call `sp-kill-region' on region or PREFIX whole lines."
    (interactive "p")
    (whole-line-or-region-wrap-region-kill 'sp-kill-region prefix))
```

and call `whole-line-or-region-sp-kill-region` (probably mapped to C-w)
to remove the whole line but only if it keeps the balance.
@dakra dakra force-pushed the make-kill-region-compatible branch from 1d3e2d3 to 142d5d7 Compare October 12, 2020 20:42
@Fuco1
Copy link
Owner

Fuco1 commented Oct 14, 2020

The first wrapper uses funcall which needs exact amount of arguments, but the beg-end wrapper uses apply which only specifies the first two positional arguments and the rest is passed as a list. If this list is nil, it won't be passed to the function and there's then no error.

So if you don't pass anything extra to the wrapper you're fine.

@Fuco1
Copy link
Owner

Fuco1 commented Oct 14, 2020

About the sponsor, it wasn't available in my country but it seems that it now is. I set it up now but it's pending approval.

@dakra
Copy link
Author

dakra commented Oct 15, 2020

OK. I'm closing this issue now as

(defun whole-line-or-region-sp-kill-region (prefix)
  "Call `sp-kill-region' on region or PREFIX whole lines."
  (interactive "*p")
  (whole-line-or-region-wrap-beg-end 'sp-kill-region prefix))

seems to work the same.
I'm still not sure what those 2 macros in the ..-kill-region
function do and why there even is another function.

But I couldn't find a difference in my one day test so far.

Thanks for your help.

@dakra dakra closed this Oct 15, 2020
@Fuco1
Copy link
Owner

Fuco1 commented Oct 22, 2020

fyi: my github sponsor profile is now live 🎉

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.

2 participants