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

Fix extglob patterns matching valid paths and update tests. #2

Merged
merged 1 commit into from
Oct 2, 2015
Merged

Conversation

danhper
Copy link
Contributor

@danhper danhper commented Oct 1, 2015

Hi,

isGlob was currently matching things like @mypath as a glob, but @mypath being a perfectly
valid path, even with extglob on, I think it shoud not match.
Here is the bash documentation about this.
https://www.gnu.org/software/bash/manual/html_node/Pattern-Matching.html

I updated to match @(my|pattern) as a glob, but not @mypath. I did the same for paths with +.

This is actually causing an issue in chokidar (which relies implicitly on this through glob-parent),
as path/to/@mydir is not treated as it should be.

Also, from should.js 0.7, should.be.true like matchers all became functions so I modified the tests,
otherwise they would become noop as the version of should.js was not locked.

Thanks.

@jonschlinkert
Copy link
Member

hmm, I see your point. I think if we add this functionality we should just use: https://github.com/jonschlinkert/is-extglob. It's more comprehensive and has extensive unit tests for extglob features.

would you mind updating the pr to just use is-extglob?

@danhper
Copy link
Contributor Author

danhper commented Oct 2, 2015

Hi, thanks for the quick reply.
I did not know you made a package for this, but sure it is better to use something well tested!
I just updated the PR.

@jonschlinkert
Copy link
Member

great thanks!

jonschlinkert added a commit that referenced this pull request Oct 2, 2015
Fix extglob patterns matching valid paths and update tests.
@jonschlinkert jonschlinkert merged commit d7db1b2 into micromatch:master Oct 2, 2015
@jonschlinkert
Copy link
Member

published as patch release 2.0.1 on npm, and tagged as 2.0.1. thanks @tuvistavie!

@danhper
Copy link
Contributor Author

danhper commented Oct 2, 2015

Great, thanks!

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