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

feat!: do not check for expect before printing the argument of accept… #625

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

LoricAndre
Copy link
Contributor

@LoricAndre LoricAndre commented Nov 27, 2024

…(...)

Checklist

check the box if it is not applicable to your changes

  • I have updated the README with the necessary documentation
  • I have added unit tests
  • I have added end-to-end tests
  • I have linked all related issues or PRs
  • I have re-generated the completions and manpage using cargo xtask compgen and cargo xtask mangen

Description of the changes

  • Do not require --expect to be able to print the argument of the accept binding
  • Do not print an empty line if --expect is set but a different accept key is pressed
  • Effectively starts the deprecation of --expect <key> in favor of --bind:accept(<key>)

closes #624

@LoricAndre
Copy link
Contributor Author

LoricAndre commented Nov 27, 2024

@ibhagwan if you have some time, I'd love for you to take a very quick look at this and give me your opinion !

@LoricAndre LoricAndre self-assigned this Nov 27, 2024
@LoricAndre LoricAndre added this to the v0.14.0 milestone Nov 27, 2024
@LoricAndre LoricAndre merged commit bcee1f4 into master Nov 27, 2024
6 of 9 checks passed
@LoricAndre LoricAndre deleted the feat/accept-return branch November 27, 2024 19:01
@ibhagwan
Copy link

ibhagwan commented Nov 27, 2024

@ibhagwan if you have some time, I'd love for you to take a very quick look at this and give me your opinion !

Will do some testing on master and let you know my thoughts, Ty @LoricAndre!

@LoricAndre LoricAndre restored the feat/accept-return branch November 27, 2024 19:14
@LoricAndre
Copy link
Contributor Author

I'll revert the change if you see anything, don't hesitate to tell me !

@ibhagwan
Copy link

ibhagwan commented Nov 27, 2024

@LoricAndre, maybe I'm missing something but the below still doesn't work (the select-all is ignored when --expect is found):

echo "1\n2" | sk --multi --bind=ctrl-q:select-all+accept --expect=ctrl-q

image

@LoricAndre
Copy link
Contributor Author

Could you try --bind ctrl-q:select-all+accept(ctrl-q), without expect ?

@ibhagwan
Copy link

Could you try --bind ctrl-q:select-all+accept(ctrl-q), without expect ?

Works
image

If accept(<key>) the way to go no problem, I just wasn't sure if this PR should also "fix" the expect+bind combo.

@LoricAndre
Copy link
Contributor Author

I think we'll try to deprecate --expect, I don't really know why it is there since internally it is translated into and Accept event, just like the one you create with the command above.

@ibhagwan
Copy link

I think we'll try to deprecate --expect, I don't really know why it is there since internally it is translated into and Accept event, just like the one you create with the command above.

Good call, I find it confusing and counter intuitive and I like the accept(key) approach.

@LoricAndre LoricAndre deleted the feat/accept-return branch November 27, 2024 20:44
@ibhagwan
Copy link

dotdash pushed a commit to dotdash/skim that referenced this pull request Dec 7, 2024
The vim plugin uses --expect and expects the key used to accept the
selection on the first line of output. Commit bcee1f4 "feat!: do not
check for expect before printing the argument of accept… (skim-rs#625)" broke
that by not outputting anything if the selection was made using the
default "enter" key. We can workaround that by explicitly adding
"enter" to the --expect argument, so that it once again shows up in the
output.
dotdash pushed a commit to dotdash/skim that referenced this pull request Dec 7, 2024
The vim plugin uses --expect and expects the key used to accept the
selection on the first line of output. Commit bcee1f4 "feat!: do not
check for expect before printing the argument of accept… (skim-rs#625)" broke
that by not outputting anything if the selection was made using the
default "enter" key. We can workaround that by explicitly adding
"enter" to the --expect argument, so that it once again shows up in the
output.

Fixes skim-rs/skim.vim#25
@dotdash dotdash mentioned this pull request Dec 7, 2024
4 tasks
dotdash pushed a commit to dotdash/skim that referenced this pull request Dec 7, 2024
The vim plugin uses --expect and expects the key used to accept the
selection on the first line of output. Commit bcee1f4 "feat!: do not
check for expect before printing the argument of accept… (skim-rs#625)" broke
that by not outputting anything if the selection was made using the
default "enter" key. We can workaround that by explicitly adding
"enter" to the --expect argument, so that it once again shows up in the
output.

Fixes skim-rs/skim.vim#25
LoricAndre pushed a commit that referenced this pull request Dec 8, 2024
The vim plugin uses --expect and expects the key used to accept the
selection on the first line of output. Commit bcee1f4 "feat!: do not
check for expect before printing the argument of accept… (#625)" broke
that by not outputting anything if the selection was made using the
default "enter" key. We can workaround that by explicitly adding
"enter" to the --expect argument, so that it once again shows up in the
output.

Fixes skim-rs/skim.vim#25
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.

Start deprecating --expect in favor of --bind accept(key) (follow-up to #622)
2 participants