-
Notifications
You must be signed in to change notification settings - Fork 112
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
feat: 🎸 maxMatches
to limit matches and exit early
#238
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,15 +19,27 @@ export default class ReaderStream extends Reader<NodeJS.ReadableStream> { | |
const filepaths = patterns.map(this._getFullEntryPath, this); | ||
|
||
const stream = new PassThrough({ objectMode: true }); | ||
let matches = 0; | ||
|
||
stream._write = (index: number, _enc, done) => { | ||
if (options.maxMatches === matches) { | ||
// This is not ideal because we are still passing patterns to write | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Help would be appreciated here @mrmlnc - I think the current solution is okayish, however ideally we would backpressure the stream in |
||
// even though we know the stream is already finished. We can't use | ||
// .writableEnded either because finding matches is asynchronous | ||
// The best we could do is to await the write inside the for loop below | ||
// however that would mean that this whole function would become async | ||
done(); | ||
return; | ||
} | ||
|
||
return this._getEntry(filepaths[index], patterns[index], options) | ||
.then((entry) => { | ||
if (entry !== null && options.entryFilter(entry)) { | ||
stream.push(entry); | ||
matches++; | ||
} | ||
|
||
if (index === filepaths.length - 1) { | ||
if (index === filepaths.length - 1 || options.maxMatches === matches) { | ||
stream.end(); | ||
} | ||
|
||
|
@@ -37,6 +49,10 @@ export default class ReaderStream extends Reader<NodeJS.ReadableStream> { | |
}; | ||
|
||
for (let i = 0; i < filepaths.length; i++) { | ||
if (stream.writableEnded) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
break; | ||
} | ||
|
||
stream.write(i); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,5 +113,38 @@ describe('Readers → ReaderSync', () => { | |
|
||
assert.strictEqual(actual.length, 0); | ||
}); | ||
|
||
describe('maxMatches', () => { | ||
it('can be used to limit matches', () => { | ||
const reader = getReader(); | ||
const maxMatches = 2; | ||
const readerOptions = getReaderOptions({ | ||
maxMatches, | ||
entryFilter: () => true | ||
}); | ||
|
||
reader.statSync.returns(new Stats()); | ||
|
||
const actual = reader.static(['1.txt', '2.txt', '3.txt'], readerOptions); | ||
|
||
assert.strictEqual(actual.length, maxMatches); | ||
assert.strictEqual(actual[0].name, '1.txt'); | ||
assert.strictEqual(actual[1].name, '2.txt'); | ||
}); | ||
|
||
it('is ignored if less or equal than 1', () => { | ||
const reader = getReader(); | ||
const readerOptions = getReaderOptions({ | ||
maxMatches: -1, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same about |
||
entryFilter: () => true | ||
}); | ||
|
||
reader.statSync.returns(new Stats()); | ||
|
||
const actual = reader.static(['1.txt', '2.txt', '3.txt'], readerOptions); | ||
|
||
assert.strictEqual(actual.length, 3); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ export default class ReaderSync extends Reader<Entry[]> { | |
|
||
public static(patterns: Pattern[], options: ReaderOptions): Entry[] { | ||
const entries: Entry[] = []; | ||
let matches = 0; | ||
|
||
for (const pattern of patterns) { | ||
const filepath = this._getFullEntryPath(pattern); | ||
|
@@ -26,6 +27,9 @@ export default class ReaderSync extends Reader<Entry[]> { | |
} | ||
|
||
entries.push(entry); | ||
if (options.maxMatches === ++matches) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, this is an incorrect place to filter by the number of entries found. The main problem is that only static patterns are processed at this place, which, by their nature (read as So, the most correct place to filter the entries — There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. It feels like the option fits the fast-glob package, but do you suggest I use nodelib/fs-walk instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand your problem correctly (find the first file with some extension), then this package ( The filters I mentioned above need to be updated in fast-glob. |
||
break; | ||
} | ||
} | ||
|
||
return entries; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,6 +104,13 @@ export type Options = { | |
* @default false | ||
*/ | ||
markDirectories?: boolean; | ||
/** | ||
* Exit after having gathered `maxMatches` matches. | ||
* If given, expects a positive number greater or equal to 1. | ||
* | ||
* @default Infinity | ||
*/ | ||
maxMatches?: number; | ||
/** | ||
* Returns objects (instead of strings) describing entries. | ||
* | ||
|
@@ -165,6 +172,9 @@ export default class Settings { | |
public readonly globstar: boolean = this._getValue(this._options.globstar, true); | ||
public readonly ignore: Pattern[] = this._getValue(this._options.ignore, [] as Pattern[]); | ||
public readonly markDirectories: boolean = this._getValue(this._options.markDirectories, false); | ||
// If 0 or negative maxMatches is given, we revert to infinite matches | ||
// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we remove this if I:
let me know what's preferred :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO, the first option looks good to me because this logic is already exist for the |
||
public readonly maxMatches: number = Math.max(0, this._getValue(this._options.maxMatches, Infinity)) || Infinity; | ||
public readonly objectMode: boolean = this._getValue(this._options.objectMode, false); | ||
public readonly onlyDirectories: boolean = this._getValue(this._options.onlyDirectories, false); | ||
public readonly onlyFiles: boolean = this._getValue(this._options.onlyFiles, true); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import * as smoke from './smoke'; | ||
|
||
smoke.suite('Smoke → MarkDirectories', [ | ||
joscha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this test does not work as expected. This behaviour related to the comment about wrong place for filtering. You can add
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't able to run this one - it seemed to error on my machine |
||
pattern: 'fixtures/**/*', | ||
fgOptions: { maxMatches: 1 } | ||
} | ||
]); |
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.
Looks like we need check here
Infinity
instead of-1
, because-1
will be converted to theInfinity
in the Settings.