Skip to content

Allow multiple star-star globs in one pattern#20639

Merged
weswigham merged 2 commits into
microsoft:masterfrom
weswigham:allow-more-globs
Dec 13, 2017
Merged

Allow multiple star-star globs in one pattern#20639
weswigham merged 2 commits into
microsoft:masterfrom
weswigham:allow-more-globs

Conversation

@weswigham
Copy link
Copy Markdown
Member

Fixes #16115

The original issue was because when we saw a pattern like /foo, we appended /**/* to the end to build a correct regex. Now, if the input pattern was **/foo, we'd do the same (with no error, since there weren't multiple star-star globs in the original pattern), except then we'd bail out on manufacturing the regex when we saw there were multiple ** patterns (since we added /**/* to the end).

Now, I can't figure out why you'd forbid multiple **'s in a glob pattern in the first place (node-glob and bash seem to handle it just fine), so I just removed the restriction (and the early-bail clause in the regex generation code). I looked at the history, and the restriction's been in place since @rbuckton 's first iteration of the feature, so I think that it's still here was just a holdover from before we built a regex from the pattern.

Everything that's not a change to the tests in this PR is just removing lines of code, so this seems like a pretty good change!

@weswigham weswigham requested review from a user, rbuckton and sheetalkamat December 12, 2017 00:22
Comment thread src/harness/unittests/matchFiles.ts Outdated
it("can include dirs whose pattern starts with **", () => {
const json = {
include: [
"**/x"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If we use **/x/**/y will it include /a/x/b/y/c/z.ts?

@weswigham weswigham merged commit 2c6501d into microsoft:master Dec 13, 2017
@weswigham weswigham deleted the allow-more-globs branch December 13, 2017 20:57
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants