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 reject-paths parameter for skipping certain directories from traversing. #73

Closed
wants to merge 4 commits into from

Conversation

xraw
Copy link

@xraw xraw commented Oct 14, 2016

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.

add a reject-paths parameter to skip diving into paths with certain names.
reject-paths is a list of strings. useful for scanning source directories and skipping
.git/, build/ etc.
added test.
order of list items matters in the test.
@rejeep
Copy link
Owner

rejeep commented Oct 17, 2016

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
Copy link
Author

xraw commented Oct 18, 2016

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
Copy link
Owner

rejeep commented Oct 23, 2016

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
Copy link
Author

xraw commented Nov 29, 2016

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
Copy link
Owner

rejeep commented Nov 29, 2016

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
Copy link

mattiasb commented Jan 16, 2017

@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
Copy link
Owner

rejeep commented Jan 16, 2017

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
Copy link

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
Copy link
Owner

rejeep commented Feb 16, 2017

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.

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.

None yet

3 participants