-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Subtree toggle and cycle #28
Conversation
Very cool! I had this feature in my notes for a long time. I'll try to review it today evening. Another cool thing would be to have an option to not expand certain directories, like We can then provide some filters (like "filter by name") but still leave the user option to add/remove their own filters. Do you want to try to add such functionality to this PR? I'm merging it either way though, good job :) |
So I've reviewed the patch and added a couple comments, mostly minor issues. Once you fix those I'll be happy to merge this in :). |
I updated code to match your coding style and fixed few typos. I also added support for ignored files (defaults to |
(when (and name (file-directory-p name) | ||
(<= depth (or max-depth depth)) | ||
(or (= 1 depth) | ||
(not (string-match dired-subtree-ignored-regexp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use string-match-p
, the non -p
version modifies the global match-data
which might interfere with other packages. It is always better to use this if you don't care about the match data. (also fix the indent level :)
Awesome, two more little issues and it's ready. Thanks a bunch. |
Actually three issues. Should I squash patches? |
Yes, please. |
3216d0e
to
cb25296
Compare
Ready. |
I only get the expantion down to 1 level and hide, the recursive one is never hit in the cond. I think the checks need to be reordered somehow. Also, the variable is called |
You are right for the mismatch in variable name (typo). About cycle not working. How do you execute I bound it to When I replaced Maybe you use |
I use There's some variable |
Same results with Other way I can think of is to use So do you think fixing is worth the effort? I cannot imagine somebody using |
Agreed. Okey, I think we've solved everything then. |
Thanks again. |
The patch adds two features: