-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Remove with-ivy-window
in actions
#3049
base: master
Are you sure you want to change the base?
Conversation
`with-ivy-window` wrapper is legacy for actions.
This PR only contains repeated removal of
|
One thing this PR does not cover is the outdated description of the usage of |
That commit says:
But in fact, the first part of I'm inclined to bring back the norecord argument. Do you foresee any issues with that? Either way, thanks for the careful analysis so far! As you can tell I'm still double checking the history and details of this part of the code. |
Yes, that's my understanding. |
I can take care of these changes...
...unless you prefer to get started on the CA process? |
What about these two? Lines 239 to 245 in 2a25a6f
Lines 1514 to 1517 in 2a25a6f
|
I don't see potential issues from my perspective. Maybe you can push a separate commit to this PR?
I think I would avoid the lengthy paperwork unless absolutely necessary. So I will leave this part to you, thanks!
These were missed. Thanks for the quick review and I will update the PR. |
b4ff92a
to
5428012
Compare
`with-ivy-window` wrapper is legacy for actions.
5428012
to
d623372
Compare
The
with-ivy-window
wrapper became redundant in actions after 55a90c9. Its usage was subsequently replaced by(select-window (ivy--get-window ivy-last))
in 1c09e99. Its obselote status was also confirmed by abo-abo in #1727 (comment).This PR removes all
with-ivy-window
usages in actions inswiper.el
andcounsel.el
, which cleans up the code and ensures the fix in 1c09e99 is correctly applied to all actions.