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

Add walker options and replace 'find' with the built-in walker #3649

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

junegunn
Copy link
Owner

@junegunn junegunn commented Feb 29, 2024

Close #3464
Close #3647
Related #2705

Add options for the built-in directory walker and use them to replace the default 'find' commands in the shell extensions.

Option Description Default
--walker=OPTS Walker options ([file][,dir][,follow][,hidden]) file,follow,hidden
--walker-root=DIR Root directory from which to start walker .
--walker-skip=DIRS Comma-separated list of directory names to skip .git,node_modules

Note

Ways to implement --walker-skip

Benchmark

image

We use simple nested for loop comparison because the size of the list is unlikely to exceed 5.

@junegunn junegunn self-assigned this Feb 29, 2024
@junegunn junegunn marked this pull request as ready for review February 29, 2024 10:28
@junegunn junegunn changed the title Add --walker=OPTS and --walker-root=DIR and use them to replace 'find' Add walker options and replace 'find' with the built-in walker Mar 1, 2024
@junegunn junegunn force-pushed the devel branch 2 times, most recently from 93b2236 to 2616cbb Compare March 6, 2024 15:07
@junegunn
Copy link
Owner Author

junegunn commented Mar 6, 2024

I'm wondering if we should add node_modules to the default of --walker-skip. This won't cover the whole other types of projects and not everyone works with JavaScript projects, but still. Would anyone want to look inside node_modules?

Or should we try to implement .gitignore support? Is it too much? Does it really make sense to skip all the things in .gitignore? Sometimes I add a directory to .gitignore so that it's not checked in, but I still want to work with the files in it.

@alex-courtis
Copy link

Alex from nvim-tree here, please ignore me if I'm talking out of place.

I'm wondering if we should add node_modules to the default of --walker-skip. This won't cover the whole other types of projects and not everyone works with JavaScript projects, but still. Would anyone want to look inside node_modules?

node_modules have been particularly problematic for us and we recommend that users ignore them and gitignore them.

We should have ignored node_modules by default from the start, also .git.

Could I suggest that you ignore build as well as target? Meson and ninja write a lot in there, mostly boilerplate and repeated files.

@timhillgit
Copy link

I'm wondering if we should add node_modules to the default of --walker-skip.

I think that's safe. Lots of node developers out there and I don't think looking in node_modules would normally be useful.

Or should we try to implement .gitignore support? Is it too much?

I think it's probably too much for a default, like you I do sometimes want to search for files that are gitignored. Better to recommend using rg for that case.

Could I suggest that you ignore build as well as target?

Similarly, I think this is probably too broad. Without any other context they're too common of words and might appear in other places.

@junegunn
Copy link
Owner Author

junegunn commented Mar 7, 2024

Thanks a lot for the comments.

@alex-courtis

Could I suggest that you ignore build as well as target?

I also thought about that. target is particularly problematic on Maven projects. There are a bunch of .class files you never need to browse through. However, the directory also holds the final JAR artifacts that you do care about, so I'm a bit torn on this.

@timhillgit

Similarly, I think this is probably too broad. Without any other context they're too common of words and might appear in other places.

I think most of the users of fzf are software engineers, so they are unlikely to create directories named target or build for other purposes. But it's a fair point.

@kelleyma49
Copy link
Contributor

This is awesome, @junegunn ! Thank you for doing this. This should solve the problem that I was trying solve in #3464 . I will definitely utilize this

@timhillgit
Copy link

@junegunn Release 0.47.0 mentions these changes but this PR still needs to be merged. I'm guessing you probably knew but wanted to make sure.

@junegunn
Copy link
Owner Author

@kelleyma49 Thanks, I'm going to merge this and release a new version in the coming days. Please test this on Windows and let me know if you run into any problems.

@timhillgit Thanks, I'm aware. I was in a bit of a rush to push out some important bug fixes so I prematurely released 0.47.0 without realizing that I hadn't merged this PR. However, the release note for 0.47.0 isn't incorrect in that we have already merged fastwalk patch to the master branch. The default find command has been replaced but without the options proposed here to customize the behavior.

@junegunn junegunn changed the base branch from master to rc March 13, 2024 11:51
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.

None yet

4 participants