-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 support for globs in excluded paths #2316
Conversation
Generated by 🚫 Danger |
Looking at how to fix the test length |
I moved the alias related configuration tests to a new file here to help with the length issue |
Happy to move something else, or do that in a separate PR if needed |
Codecov Report
@@ Coverage Diff @@
## master #2316 +/- ##
==========================================
+ Coverage 91.99% 92.01% +0.02%
==========================================
Files 296 299 +3
Lines 14886 14936 +50
==========================================
+ Hits 13694 13744 +50
Misses 1192 1192
Continue to review full report at Codecov.
|
let includedPaths = included.flatMap { | ||
fileManager.filesToLint(inPath: $0, rootDirectory: rootPath) | ||
} | ||
return (pathsForPath + includedPaths).filter { | ||
!excludedPaths.contains($0) | ||
} | ||
} | ||
|
||
private func resolveGlobs(in pattern: String) -> [String] { |
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.
Do you think it'd be worth it to extract this to a new class and add some tests directly?
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.
Done
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.
I left the first test since it's about Configuration calling glob, and added some new glob ones
#endif | ||
|
||
return (0..<Int(matchCount)).compactMap { index in | ||
return String(validatingUTF8: globResult.gl_pathv[index]!) |
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.
what do you think about return globResult.gl_pathv[index].flatMap(String.init(validatingUTF8:))
just to avoid any potencial issues?
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.
Done, but FWIW in this case IMO it's better to crash since this would be breaking the API contract.
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.
Had to use it in a closure since it couldn't infer the type otherwise
92bdae3
to
144e4bf
Compare
This looks good to me from an implementation point of view. We probably should also update the README mentioning this and how to use it. |
Added |
Any other thoughts here @marcelofabri ? |
Thanks @keith! |
This does not support recursive globs, but it seems like for many cases
plain globs would be enough. This also means we don't need any libraries
to support this case.