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

[Search directory] Prevent duplicate / when fd version >= 8.4.0 #241

Merged
merged 7 commits into from
Jun 1, 2022

Conversation

kidonng
Copy link
Contributor

@kidonng kidonng commented May 30, 2022

fd 8.4.0 just released with this (long-awaited?) change:

Directories are now printed with an additional path separator at the end

This is different from how it is handled in fzf.fish, where directories are only appended a slash under strict conditions, so strict it may look like a bug. I'm saying this because I kinda think fzf.fish should have handled it like fd from the beginning.

Now onto the issue. Most programs treat duplicate slashes as one and work as intended, but it certainly does not look good. There are several possible solutions:

  • Don't do our thing if fd version is >= 8.4.0. Easiest one, also how this PR is done currently. Just one trivial problem: people may notice a slight difference of fzf.fish when they bump fd, not intuitive.
  • Follow fd and append slashes to directories all the time. May not worth the effort as the code are going to be dropped when most users upgraded to newer fd version.
  • Go out of our way and drop all slashes from fd. In other words, keep fzf.fish's behavior consistent across fd versions. This solution is here only for completeness.

Not sure about how to revise the README, maybe just leave it as is, the tip is useful nonetheless. Or maybe drop the "and the only path selected" part.

@kidonng
Copy link
Contributor Author

kidonng commented May 30, 2022

No idea why the tests failed, but as always I don't find tests interesting 😬

Copy link
Owner

@PatrickF1 PatrickF1 left a comment

Choose a reason for hiding this comment

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

In the readme, just put in parens (when fd version >= [8.4.0](https://github.com/sharkdp/fd/releases/tag/v8.4.0))

@@ -31,7 +31,7 @@ function _fzf_search_directory --description "Search the current directory. Repl
# Then, the user only needs to hit Enter once more to cd into that directory.
if test (count $file_paths_selected) = 1
set commandline_tokens (commandline --tokenize)
if test "$commandline_tokens" = "$token" -a -d "$file_paths_selected"
if test "$commandline_tokens" = "$token" -a -d "$file_paths_selected" -a (fd --version | string replace --regex --all '[^\d]' '') -lt 840
Copy link
Owner

Choose a reason for hiding this comment

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

Can you put the fd version check on its own line, line 33, and make it a boolean that you test in the if? more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A line break may be simper

@PatrickF1
Copy link
Owner

oh looks like you'll need to update the test too

@PatrickF1 PatrickF1 merged commit 4ebf002 into PatrickF1:main Jun 1, 2022
@PatrickF1
Copy link
Owner

Thanks @kidonng !

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.

2 participants