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

Update README to add _fzf_compgen_dir in addition to _fzf_compgen_path #1083

Closed
wants to merge 1 commit into from
Closed

Conversation

chrisjohnson
Copy link

As discussed here: #1067

@junegunn
Copy link
Owner

Thanks for the pull request. I think we still have some room for improvement here.

IMO, the code is not very readable. I'd prefer to present only short, easy-to-read code snippets that the users can easily understand without putting too much effort, as I don't want to encourage them to copy and paste some code without understanding it first.

Also, it does not print directories which don't have any files on them. For example, in the following case,

mkdir -p a/b/c/d
touch a/b/c/d/e

it only prints a/b/c/d skipping the intermediate directories. For that reason, I wrote a couple of filter scripts and have used them with other commands.

rg --files | only-dir
rg --files | with-dir

The actual scripts are not shorter than your awk script and they requires Ruby, but if the purpose of the README page is to show the users how they can customize the default behavior and give some examples, rather than to provide the best practices they can blindly follow, it seems to me that the shorter example using a separate filter script conveys the concept in more straightforward way although users can't simply copy the code and be done with it.

@chrisjohnson
Copy link
Author

chrisjohnson commented Oct 13, 2017

Ha, that ruby script is even quicker than the awk one, that surprises me. It also adds many dirs that the awk approach does not because it traverse back up the path chain to add intermediate dirs without files. It's still subject to missing dirs that have no files in them or in any subdir but seems better than what I had.

I like this approach better. What would you say to copying the only-dir script out of your dotfiles into a gist (for a permanent un-changing url) and linking to it and using your proposed rg --files | only-dir in the README?

Edit Now that I think of it, only-dir doesn't uniq. So I would actually say rg --files | only-dir | awk '!h[$0]++'

@junegunn
Copy link
Owner

Sounds like a plan. Let me put up a gist.

only-dir doesn't uniq

I believe it does by maintaining a unique set of visited directories. Let me know if it's not the case.

diff --git a/README.md b/README.md
index f7ebb73..dc26307 100644
--- a/README.md
+++ b/README.md
@@ -367,15 +367,26 @@ export FZF_COMPLETION_TRIGGER='~~'
 # Options to fzf command
 export FZF_COMPLETION_OPTS='+c -x'
 
-# Use ag instead of the default find command for listing candidates.
+# Use rg instead of the default find command for listing path candidates.
 # - The first argument to the function is the base path to start traversal
-# - Note that ag only lists files not directories
 # - See the source code (completion.{bash,zsh}) for the details.
+# - rg only lists files, so we use with-dir script to augment the output
 _fzf_compgen_path() {
-  ag -g "" "$1"
+  rg --files "$1" | with-dir "$1"
+}
+
+# Use rg to generate the list for directory completion
+_fzf_compgen_dir() {
+  rg --files "$1" | only-dir "$1"
 }
 ```
 
+`only-dir` and `with-dir` scripts can be found [here][dir-scripts]. They are
+written in Ruby, but you should be able to rewrite them in any language you
+prefer.
+
+[dir-scripts]: https://gist.github.com/junegunn/8c3796a965f22e6a803fe53096ad7a75
+
 #### Supported commands
 
 On bash, fuzzy completion is enabled only for a predefined set of commands

So, what do you think?

(As I write this, I noticed an issue with only-dir and with-dir. They traverse directories upwards until they find ., but the scheme doesn't work with absolute paths and they end up printing /. So I update both scripts to take an optional argument which denotes the base directory where they should stop.)

@junegunn
Copy link
Owner

Maybe we should still suggest ag since the patch for BurntSushi/ripgrep#200 is not yet included in an official release of ripgrep.

@chrisjohnson
Copy link
Author

Aha it's a Set, that makes sense then. And I agree with favoring ag over rg until that bug is fixed. Looks like a solid approach, I agree with it entirely

@junegunn junegunn closed this in cb8e972 Oct 14, 2017
@junegunn
Copy link
Owner

Updated the page in cb8e972. Thanks for the suggestions and comments!

@chrisjohnson
Copy link
Author

chrisjohnson commented Oct 14, 2017 via email

@partounian
Copy link

What is the difference with setting compgen and FZF_DEFAULT_COMMAND? Skimming through code seems completely different but description wise they seem to be described the same, so I'm confused.

@chrisjohnson
Copy link
Author

I am guessing based on my experience, so somebody else might correct me, FZF_DEFAULT_COMMAND is used when you invoke fzf the command, and the compgen functions are invoked when you're trying to use fzf to complete command line arguments with **

@junegunn
Copy link
Owner

So I updated the examples again, after trying out fd, which is a tool specifically designed for this use case. No need for extra scripts.

See: a6d2ab3

@cc978 cc978 mentioned this pull request May 29, 2020
15 tasks
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.

4 participants