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

Introduce p-ranav/glob library for file globbing #50

Merged
merged 2 commits into from
Aug 5, 2022
Merged

Conversation

timwoj
Copy link
Member

@timwoj timwoj commented Aug 2, 2022

This adds the p-ranav/glob library to replace using OS-specific glob methods, and replaces all of the Windows-API-based code in files.windows.cc with normal filesystem methods to do the same thing. The code between Windows and POSIX could potentially be combined into a single file, since they are very close to each other at this point.

There is one additional TODO I added at the error case in FilesLinesWindows::snapshot. First off, this error case probably isn't being hit by the tests. The table requires four columns but we're only inserting three here (we're missing the pattern column). I only discovered this because we fall into this if block when we hit the sub directory added to the test when it can't be opened by ifstream. I added a check for directories to get around it temporarily.

Comment on lines +58 to +59
// glob::glob returns std::filesystem::path, but we're using ghc::filesystem for compatibility
// reasons. this means we need to copy the paths from one vector type to another here.
Copy link
Member

Choose a reason for hiding this comment

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

Their README says: If you can't use C++17, you can integrate [gulrak/filesystem](https://github.com/gulrak/filesystem) with minimal effort.. I have this vague memory, that I've actually done that in the past with this library, but I don't find it right now after a quick look. Something to potentially clean up later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it could be as simple as this: p-ranav/glob#2

@rsmmr rsmmr merged commit 98560c6 into main Aug 5, 2022
@rsmmr rsmmr deleted the topic/timw/globbing branch August 5, 2022 08:30
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.

2 participants