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

Improved filter performance #44

Merged
merged 2 commits into from Nov 10, 2015
Merged

Improved filter performance #44

merged 2 commits into from Nov 10, 2015

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Nov 5, 2015

Reduced matchers computing

@TrySound
Copy link
Contributor Author

TrySound commented Nov 5, 2015

9 tests don't pass on windows in master.

@jonschlinkert
Copy link
Member

9 tests don't pass on windows in master.

with these changes or before?

@TrySound
Copy link
Contributor Author

TrySound commented Nov 5, 2015

@jonschlinkert Both

@jonschlinkert
Copy link
Member

What version of windows/node are you using? I typically run the tests and bechmarks on both windows and mac.

@TrySound
Copy link
Contributor Author

TrySound commented Nov 5, 2015

@jonschlinkert windows 7 && node 0.12 && npm 3

@TrySound
Copy link
Contributor Author

TrySound commented Nov 5, 2015

@jonschlinkert
Copy link
Member

Thanks for the additional info! I'll take a look at this asap. just wanted you to know I didn't forget

@TrySound
Copy link
Contributor Author

TrySound commented Nov 6, 2015

Thanks, Jon

@@ -150,7 +155,7 @@ function filter(patterns, opts) {

fp = utils.unixify(fp, opts);
while (i < len) {
var fn = matcher(patterns[i++], opts);
var fn = patterns[i++];
if (!fn(fp)) {
res = false;
Copy link
Member

Choose a reason for hiding this comment

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

These are great changes. nice work.

@es128 any objections to merging? I was going to do a 2.4.0 patch release with housekeeping stuff, like changing to eslint, but maybe we just merge this and do 2.4.0. then I can follow up with a 2.5.0. sound ok?

Copy link
Member

Choose a reason for hiding this comment

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

No substantial objections to the change, but if there's no change to API footprint why not bump patch instead of minor?

Also would still like to see tests added for the backslash scenarios, and generally it's safer to run anymatch and chokidar tests on any patch before release just in case.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind my comment about the backslash tests, just noticed you already did that. 👍

Copy link
Member

Choose a reason for hiding this comment

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

why not bump patch instead of minor?

of course lol, I was pretty tired when I wrote that. seems I was looking for the most complicated way to do things possible lol

@es128
Copy link
Member

es128 commented Nov 9, 2015

Tests passing now. LGTM

@TrySound
Copy link
Contributor Author

TrySound commented Nov 9, 2015

Squash it?

@es128
Copy link
Member

es128 commented Nov 9, 2015

Up to @jonschlinkert... I don't think it's needed in this case, personally.

@jonschlinkert
Copy link
Member

all is good, thanks!

jonschlinkert added a commit that referenced this pull request Nov 10, 2015
@jonschlinkert jonschlinkert merged commit 9d8043c into micromatch:master Nov 10, 2015
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.

None yet

3 participants