-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Resolve basePath first #67
Conversation
@@ -188,4 +187,11 @@ function globIsSingular(glob) { | |||
}); | |||
} | |||
|
|||
function getBasePath(ourGlob, opt) { | |||
if (opt.base) { |
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.
this check should happen in createStream, use a ternary on L18
Seems good, @phated ? |
Thanks @contra. I moved the |
@@ -61,6 +61,25 @@ describe('glob-stream', function() { | |||
}); | |||
}); | |||
|
|||
it('should handle ( ) in directory paths', function(done) { | |||
var cwd = join(__dirname, './fixtures/has (parens)'); |
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 happens if the cwd
is ./fixtures
and you use **/*.dmc
as the glob? Maybe that should be another test
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 can add more tests or update some of the existing ones that already test that to include the test.dmc
.
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.
Either works for me. I just wanted to try to find other scenarios that might not be tested currently.
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. Added and updated a couple of the tests.
LGTM. @doowb it's really awesome and very much appreciated that you can jump on issues like these and get them patched up (throughout the gulpjs org). |
Thanks @phated, I'm glad I can help. |
This PR resolves the
basePath
before expanding the glob into an absolute path. This is necessary for when paths contain "glob like" characters but aren't part of the actual glob pattern.I wanted to get this up here for review. I'll be able to add a test for the specific gulp issue later today.