Avoid creating multiple Minimatch instances for the same negative glob #33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(improved a comment & squashed) |
UltCombo
changed the title
|
Changed commit message. |
UltCombo
added a commit
to sindresorhus/globby
that referenced
this pull request
Jan 1, 2015
This optimizes use cases such as `['a', 'b', '!b']`, where a new Minimatch instance for `'!b'` would be created for each previous positive glob. Now the Minimatch instances for negative globs are created beforehand, thus positive globs can use the same negative matcher instances. That is, for `['a', 'b', '!b']` both `'a'` and `'b'` globs will use the same Minimatch instance for `'!b'` instead of instantiating their own. Should also note that this change, technically, would instantiate Minimatch instances that would never be used when considering leading negative globs (e.g. `['!b', 'b']`), but that is a complete misuse and IIRC Minimatch produces their underlying regex lazily, so this shouldn't be a problem. (imported from gulpjs/glob-stream#33)
yocontra
pushed a commit
that referenced
this pull request
Jan 2, 2015
Avoid creating multiple Minimatch instances for the same negative glob
|
Nice catch - thanks! |
|
No problem, though, I believe it lost a bit of readability, seeing as negatives' |
UltCombo
added a commit
to UltCombo/glob-stream
that referenced
this pull request
Jan 2, 2015
- Better naming for negative matchers ( gulpjs#33 (comment) ) - Throw on invalid glob argument ( gulpjs#34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does. - Applied glob type validation in the `isNegative` function, as all globs go through it before stream creation and that function is already testing their types.
UltCombo
added a commit
to UltCombo/glob-stream
that referenced
this pull request
Jan 2, 2015
- Better naming for negative matchers ( gulpjs#33 (comment) ) - Throw on invalid glob argument ( gulpjs#34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
UltCombo
added a commit
to UltCombo/glob-stream
that referenced
this pull request
Jan 2, 2015
- Better naming for negative matchers ( gulpjs#33 (comment) ) - Throw on invalid glob argument ( gulpjs#34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
UltCombo
added a commit
to UltCombo/glob-stream
that referenced
this pull request
Jan 2, 2015
- Better naming for negative matchers ( gulpjs#33 (comment) ) - Throw on invalid glob argument ( gulpjs#34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
phated
pushed a commit
that referenced
this pull request
Feb 16, 2017
Avoid creating multiple Minimatch instances for the same negative glob
phated
pushed a commit
that referenced
this pull request
Feb 16, 2017
- Better naming for negative matchers ( #33 (comment) ) - Throw on invalid glob argument ( #34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
phated
pushed a commit
that referenced
this pull request
Feb 16, 2017
Avoid creating multiple Minimatch instances for the same negative glob
phated
pushed a commit
that referenced
this pull request
Feb 16, 2017
- Better naming for negative matchers ( #33 (comment) ) - Throw on invalid glob argument ( #34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
phated
pushed a commit
that referenced
this pull request
Feb 16, 2017
- Better naming for negative matchers ( #33 (comment) ) - Throw on invalid glob argument ( #34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
phated
pushed a commit
that referenced
this pull request
Feb 16, 2017
- Better naming for negative matchers ( #33 (comment) ) - Throw on invalid glob argument ( #34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
phated
pushed a commit
that referenced
this pull request
Feb 20, 2017
- Better naming for negative matchers ( #33 (comment) ) - Throw on invalid glob argument ( #34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
phated
pushed a commit
that referenced
this pull request
Feb 21, 2017
- Better naming for negative matchers ( #33 (comment) ) - Throw on invalid glob argument ( #34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
phated
pushed a commit
that referenced
this pull request
Feb 21, 2017
- Better naming for negative matchers ( #33 (comment) ) - Throw on invalid glob argument ( #34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
phated
pushed a commit
that referenced
this pull request
Feb 21, 2017
- Better naming for negative matchers ( #33 (comment) ) - Throw on invalid glob argument ( #34 (comment) ) - Removed the `if (!Array.isArray(globs)) return gs.createStream(globs, [], opt);` because this optimization is already covered a couple lines below at `if (positives.length === 1) return streamFromPositive(positives[0]);`, and to not duplicate the validation code. Previously, passing a string with a negative glob did not throw a "Missing positive glob" error, now it does.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This optimizes use cases such as
['a', 'b', '!b'], where a new Minimatch instance for'!b'would be created for each previous positive glob.Now the Minimatch instances for negative globs are created beforehand, thus positive globs can use the same negative matcher instances. That is, for
['a', 'b', '!b']both'a'and'b'globs will use the same Minimatch instance for'!b'instead of instantiating their own.Also, as far as I can see,
gs.createStreamis internal (no docs or tests), so I took the liberty of adapting it a bit as I needed the completeoptobject to pass to the Minimatch constructor ings.create.Should also note that this change, technically, would instantiate Minimatch instances that would never be used when considering leading negative globs (e.g.
['!b', 'b']), but that is a complete misuse and IIRC Minimatch produces their underlying regex lazily, so this shouldn't be a problem.