Skip to content

Scandir improvements#169

Merged
Conni2461 merged 6 commits intonvim-lua:masterfrom
jose-elias-alvarez:scandir-improvements
Sep 1, 2021
Merged

Scandir improvements#169
Conni2461 merged 6 commits intonvim-lua:masterfrom
jose-elias-alvarez:scandir-improvements

Conversation

@jose-elias-alvarez
Copy link
Copy Markdown

@jose-elias-alvarez jose-elias-alvarez commented Jun 15, 2021

This PR combines a handful of small fixes and features for the scandir module as well as a refactor of the respect_gitignore functionality. Instead of attempting to interpret the .gitignore file, which is quite difficult when the file contains more advanced patterns, it instead checks relative file paths against the output of git ls-tree -rt HEAD --name-only. This should close #76.

I've split up the changes into separate commits for easier review but will squash them whenever this PR is ready to merge.

@jose-elias-alvarez
Copy link
Copy Markdown
Author

I also wanted to take care of #75 as part of this PR but noted that changing search_pattern as described in the issue breaks the relevant test, so to clarify: do we want search_pattern to match only against the file / directory name and not the full path?

@Conni2461
Copy link
Copy Markdown
Collaborator

What? Why? What? If we want to get the output of git ls-tree -rt HEAD --name-only sync why would we even want to call scan_dir and iterate the filesystem ourselves? We then could just return git ls-tree -rt HEAD --name-only and be done. No need to compare anything.

I just done get it right now. I know its hard. So i would look at already existing implementations like https://github.com/ggreer/the_silver_searcher/blob/master/src/ignore.c or https://github.com/BurntSushi/ripgrep/tree/master/crates/ignore

@jose-elias-alvarez
Copy link
Copy Markdown
Author

Understood. Frankly, my Lua skills aren't up to the task of creating an implementation on the level of rg's or ag's, but the implementation I just pushed should bring gitignore handling closer to what's specified in the documentation. Let me know how it looks.

@jose-elias-alvarez
Copy link
Copy Markdown
Author

@Conni2461 Let me know if there's anything I can add / change on this to get it merged!

Comment thread lua/plenary/scandir.lua
Copy link
Copy Markdown
Collaborator

@Conni2461 Conni2461 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, sorry i hadn't have much time to review it and now tami knows about it and forces me to merge it 😆

I was thinking, maybe we could also expose the git ignore things as scandir.__ functions and then test these things isolated. Thoughts

@jose-elias-alvarez
Copy link
Copy Markdown
Author

jose-elias-alvarez commented Jul 17, 2021

Thanks for taking a look at this again! I exposed make_gitignore and added tests asserting against the new functionality. Let me know how everything looks.

@tjdevries
Copy link
Copy Markdown
Member

Sorry, I haven't been reviewing much lately. If you fix conflicts I will take a look again and we can merge after I review

@jose-elias-alvarez
Copy link
Copy Markdown
Author

Thanks! I just fixed the conflicts (and formatted) so let me know. Not sure what's going on with the test failures, but the scandir tests seem fine on my end, and I'm getting the same issues on the master branch.

@Conni2461
Copy link
Copy Markdown
Collaborator

Everything look good here thanks :) can you do another rebase to make sure that the tests are happy. I think all of those issues are solved in master but just in case 😆 I keep this tab open. I intend to merge this today :)

@Conni2461
Copy link
Copy Markdown
Collaborator

I just run it locally. Everything seems to work. Thanks you very much :)

Sorry for neglecting the PR

@Conni2461 Conni2461 merged commit 06266e7 into nvim-lua:master Sep 1, 2021
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.

plenary.scandir respect_gitignore doesn't work with more complex gitignores

3 participants