rejeep / f.el

Modern API for working with files and directories in Emacs
GNU General Public License v3.0
685 stars 69 forks source link

Add reject-paths parameter for skipping certain directories from traversing. #73

Closed xraw closed 7 years ago

xraw commented 8 years ago

Extend f--collect-entries with an optional reject-paths argument for ignoring certain folders from being traversed. Useful when scanning through source code folders which contain .git/, .svn/, .gradle/, build/ folders.

rejeep commented 8 years ago

I don't really see the point of this? Why not use the fn argument? For example, your test would be:

(--map
 (f-relative it "foo")
 (f-entries "foo"
            (lambda (path)
              (-contains? (list "ignore_inner") (f-filename path)))
            t))
xraw commented 8 years ago

The (-contains? ...) expression should be wrapped with (not (-contains? ...)) in order to receive the list of entries excluding the stuff in list, on match.

Furthermore, when I use the fn arg with the above mentioned test, f-entries does not traverse sub folders, I am examining why.

The idea of my change is to cut off any traversal of certain entries in order to speed up things.

rejeep commented 8 years ago

I see your point. The way it work now is that all entries are added to a list and then a filter is applied to that list.

Instead of adding the extra argument, like you suggest in this PR, how about improving the existing "fn" argument (and also rename, it's a bad name) so that it does not traverse when fn returns false.

What do you think?

xraw commented 7 years ago

Sorry for the late reply. I did a couple tests with the given/suggested approach but all of them are way slower than the reject-paths approach. I tested it on a huge android project containing .git/, multiple build/ folders, all in all over thousands of files.

The main problem is that f--collect-entries is called first and then the filtering (as you already wrote). As far as I understand the code, fn is called AFTER collecting.

Background: I needed a quick replacement for the projectile C-c p f and C-c p d commands on windows systems and saw that I could easily do it with your package and ido - speeding it up like on GNU/Linux.

If you still think it is not required, I suggest we close this request, no worries.

rejeep commented 7 years ago

I guess you can do it the way I suggest with good performance, but it might be a bit complex. If you find a good solution to it, reopen this, closing for now.

mattiasb commented 7 years ago

@rejeep your proposed solution would change the behaviour of f-entries though, potentially breaking code depending on f.

I'd suggest instead renaming the recursive argument to traverse throughout the code and if it's a defun treat it as a predicate for whether a directory should be traversed and if it's not instead treat it like the recursive boolean we have today.

What do you think?

rejeep commented 7 years ago

If the name does not change, I don't think it would break the API? The only difference is that one is "lazy" and the other is not.

mattiasb commented 7 years ago

Yeah, no API break but a semantic change of f-entries.

Say you have a directory structure like this:

mattiasb@DESKTOP-42H9U7U /mnt/c/Users/mattias.bengtsson $ tree test
test
└── lol
    └── readme.md

And you call f-entries like this:

*** Welcome to IELM ***  Type (describe-mode) for help.
ELISP> (require 'f)
f
ELISP> (f-entries "~/test/" (lambda (d) (not (equal (f-filename d) "lol"))) t)
("c:/Users/mattias.bengtsson/test/lol/readme.md")

Whereas with your proposal you'd get this:

*** Welcome to IELM ***  Type (describe-mode) for help.
ELISP> (require 'f)
f
ELISP> (f-entries "~/test/" (lambda (d) (not (equal (f-filename d) "lol"))) t)
nil
rejeep commented 7 years ago

Right. Even though it's a breaking change, this might be the way to go. The current semantic could work just as well with a f-entries plus a -reject.