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 recursive glob support #636

Merged
merged 6 commits into from
Sep 1, 2019
Merged

Add recursive glob support #636

merged 6 commits into from
Sep 1, 2019

Conversation

bclymer
Copy link
Collaborator

@bclymer bclymer commented Aug 28, 2019

Fixes #180.

Glob.swift comes from R.swift https://github.com/mac-cain13/R.swift/blob/master/Sources/RswiftCore/Util/Glob.swift, which was previously taken from other OS projects. Credits are provided in the header.

Glob itself comes with numerous tests, but I was also able to activate the previously commented out recursive globstar tests in XcodeGen. In addition, I added another scenario of a recursive globstar in a subdirectory, ensuring it does not match all directories, only those specified.

@brentleyjones brentleyjones changed the title Add recursive glob support. Add recursive glob support Aug 28, 2019
@brentleyjones
Copy link
Collaborator

🤤

I've been waiting for this.

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you @bclymer! This will make many people happy 😃
Could you please add the following:

  • a changelog entry
  • a comment in the ProjectSpec docs to say what type of glob we support. Looking at the imported code it seems to be BashV4?
  • add some examples of recursive globs in the ProjectSpec doc excludes example

return urls
}

private func listFilePaths(pattern: String) -> [String] {
Copy link
Owner

Choose a reason for hiding this comment

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

This class already has too many functions (needs a refactor). For now can we just combine these two functions into one? Our basic currency type is always Path

@acecilia
Copy link
Collaborator

Thanks! @bclymer would it be possible to have the Glob code and tests as an independent Github project and swift package, so instead of having the source code in the project we can just add it to the Package.swift manifest?

@bclymer
Copy link
Collaborator Author

bclymer commented Aug 31, 2019

@yonaskolb sorry for the delay, I made the changes you requested.

@acecilia While I think that's a good idea, it feels a bit strange to me since it's not actually my code. Tracing back the links in the header comment it looks like it's been adapted from a number of gists, and then it moved to R.swift and has been modified there since.

@yonaskolb
Copy link
Owner

yonaskolb commented Aug 31, 2019

Thanks @bclymer! I just noticed some errors in the log from the new glob tests:

Error parsing file system item: Error Domain=NSCocoaErrorDomain Code=260 "The folder “dir2” doesn’t exist." UserInfo={NSFilePath=/tmp/glob-test.XBQ0m/dir2/, NSUserStringVariant=(
    Folder
), NSUnderlyingError=0x7fa8cdb00f20 {Error Domain=NSOSStatusErrorDomain Code=-43 "fnfErr: File not found"}}

https://github.com/yonaskolb/XcodeGen/pull/636/checks?check_run_id=208818534

Could we remove these errors

@bclymer
Copy link
Collaborator Author

bclymer commented Aug 31, 2019

@yonaskolb seeing that locally as well. I just didn't scroll up enough to see it originally. I'll get it resolved.

@acecilia
Copy link
Collaborator

@bclymer okey. I also think is a good idea, since I have seen already several projects rolling their own Glob implementations.

@bclymer
Copy link
Collaborator Author

bclymer commented Aug 31, 2019

Pushed a fix. The error was semi-intentional. It was testing a double globstar (tempDir/**/dir2/**/*), which evaluates all found directories in tempDir, but ** also means nonexistent, so it tested tempDir/dir2/**/* as well.

I've mainly silenced the warning by just checking if the path exists first, and then breaking out that rather large directory iteration block into it's own function.

@yonaskolb yonaskolb merged commit 88fb6f2 into yonaskolb:master Sep 1, 2019
@yonaskolb yonaskolb mentioned this pull request Sep 1, 2019
@rvi rvi mentioned this pull request Jun 11, 2020
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.

Support for recursive Source excludes
4 participants