feat: 🎸 `maxMatches` to limit matches and exit early #238

joscha · 2019-11-05T03:15:52Z

What is the purpose of this pull request?

This introduces a new option maxMatches which can be used to limit the
number of matches that fast-glob returns.

What changes did you make? (Give an overview)

Added a maxMatches option
Added early exits to the stream and sync interface

Closes: #225

This introduces a new option `maxMatches` which can be used to limit the number of matches that fast-glob returns. Closes: mrmlnc#225

joscha · 2019-11-05T03:29:35Z

src/settings.ts

@@ -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


can we remove this if I:

ignore negative and zero maxMatches

introduce a new class method that can bound maxMatches between 1 and Infinity

assert maxMatches to be gte 1 in the constructor

let me know what's preferred :)

IMHO, the first option looks good to me because this logic is already exist for the deep and concurrency options.

joscha · 2019-11-05T03:32:05Z

src/readers/stream.ts


 		stream._write = (index: number, _enc, done) => {
+			if (options.maxMatches === matches) {
+				// This is not ideal because we are still passing patterns to write


Help would be appreciated here @mrmlnc - I think the current solution is okayish, however ideally we would backpressure the stream in for (let i = 0; i < filepaths.length; i++) { based on the async stream.write task. Happy to follow another route as well if you have ideas.

src/tests/smoke/max-matches.smoke.ts

mrmlnc

There are some issues here that needs to work through.

If you abstract from fast-glob, the @nodelib/fs.walk package is not the solution to your problem? The fast-glob package built on this package. You can write your own shared filter.

mrmlnc · 2019-11-08T08:50:25Z

src/readers/sync.ts

@@ -26,6 +27,9 @@ export default class ReaderSync extends Reader<Entry[]> {
 			}

 			entries.push(entry);
+			if (options.maxMatches === ++matches) {


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 filepath), will always be in a single instance. There are two types of patterns in the fast-glob package: static and dynamic.

So, the most correct place to filter the entries — filters. You can use the unique filter as example. The logic is simple — you need to stop reading the directory in depth, as soon as the number of found entries exceeds a specified number in the maxMatches option. But even in this case we have a problem — we need to synchronize the number of found entries between filters (deep — directories, entries — directories, symbolic links and files). Right now I don't see a good solution for this problem.

Okay. It feels like the option fits the fast-glob package, but do you suggest I use nodelib/fs-walk instead?

If I understand your problem correctly (find the first file with some extension), then this package (@nodelib/fs.walk) seems to suit you more (just add filter function with includes). If you need patterns, then, yes, here you need fast-glob.

The filters I mentioned above need to be updated in fast-glob.

mrmlnc · 2019-11-08T08:56:13Z

src/readers/stream.spec.ts

+			it('is ignored if less or equal than 1', (done) => {
+				const reader = getReader();
+				const readerOptions = getReaderOptions({
+					maxMatches: -1,


Looks like we need check here Infinity instead of -1, because -1 will be converted to the Infinity in the Settings.

mrmlnc · 2019-11-08T08:57:32Z

src/readers/sync.spec.ts

+			it('is ignored if less or equal than 1', () => {
+				const reader = getReader();
+				const readerOptions = getReaderOptions({
+					maxMatches: -1,


The same about -1 and Infinity.

mrmlnc · 2019-11-08T09:00:08Z

src/settings.ts

@@ -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


IMHO, the first option looks good to me because this logic is already exist for the deep and concurrency options.

mrmlnc · 2019-11-08T09:21:04Z

src/readers/stream.ts

@@ -37,6 +49,10 @@ export default class ReaderStream extends Reader<NodeJS.ReadableStream> {
 		};

 		for (let i = 0; i < filepaths.length; i++) {
+			if (stream.writableEnded) {


The writableEnded property exist in the Stream only start with Node.js 12.9.0+. The fast-glob package supports Node.js 8+. This works now because writableEnded has undefined value.

mrmlnc · 2019-11-08T09:27:37Z

src/tests/smoke/max-matches.smoke.ts

+import * as smoke from './smoke';
+
+smoke.suite('Smoke → maxMatches', [
+	{


Looks like this test does not work as expected. This behaviour related to the comment about wrong place for filtering.

You can add debug: true to the test and the console will contain the following lines:

Smoke → maxMatches FIXTURES NODE_GLOB FAST_GLOB ---------------------------------------- --------- --------- fixtures/file.md + + fixtures/first + + fixtures/first/file.md + + fixtures/first/nested + + fixtures/first/nested/directory + + fixtures/first/nested/directory/file.md + + fixtures/first/nested/file.md + + fixtures/second + + fixtures/second/file.md + + fixtures/second/nested + + fixtures/second/nested/directory + + fixtures/second/nested/directory/file.md + + fixtures/second/nested/file.md + + √ pattern: 'fixtures/**/*' (sync) (17ms)

I wasn't able to run this one - it seemed to error on my machine

joscha · 2019-11-08T13:15:45Z

If you abstract from fast-glob, the @nodelib/fs.walk package is not the solution to your problem?

Just to clarify: if I use the streaming interface (https://github.com/mrmlnc/fast-glob/#stream) and I was only interested in the first match, using this code:

          const stream = fg.stream(glob);
          for await (const entry of stream) {
            return true; // first match ends the stream
          }

that would work, right?

mrmlnc · 2019-11-08T15:51:50Z

that would work, right?

Good catch! The stream will not be destroyed right now. And… this is a bug. Will be fixed in issue #239.

joscha · 2019-11-10T22:23:36Z

closing this in favour of #238 (comment)

joscha added 3 commits November 5, 2019 14:14

feat: 🎸 maxMatches to limit matches and exit early

4d06b65

This introduces a new option `maxMatches` which can be used to limit the number of matches that fast-glob returns. Closes: mrmlnc#225

fix: do not write to stream if stream is ended

f6aa06e

style: 💄 lints

2a94630

joscha commented Nov 5, 2019

View reviewed changes

style: 💄 fix lints

7a44592

joscha commented Nov 5, 2019

View reviewed changes

src/tests/smoke/max-matches.smoke.ts Outdated Show resolved Hide resolved

test: fix name

c1737ea

mrmlnc self-requested a review November 6, 2019 12:09

mrmlnc self-assigned this Nov 6, 2019

mrmlnc requested changes Nov 8, 2019

View reviewed changes

mrmlnc mentioned this pull request Nov 8, 2019

Stream is not closed when the receiver is closed #239

Closed

joscha closed this Nov 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🎸 `maxMatches` to limit matches and exit early #238

feat: 🎸 `maxMatches` to limit matches and exit early #238

joscha commented Nov 5, 2019

joscha Nov 5, 2019

mrmlnc Nov 8, 2019

joscha Nov 5, 2019

mrmlnc left a comment •

edited

mrmlnc Nov 8, 2019

joscha Nov 8, 2019

mrmlnc Nov 8, 2019 •

edited

mrmlnc Nov 8, 2019

mrmlnc Nov 8, 2019

mrmlnc Nov 8, 2019

mrmlnc Nov 8, 2019

mrmlnc Nov 8, 2019

joscha Nov 8, 2019

joscha commented Nov 8, 2019

mrmlnc commented Nov 8, 2019

joscha commented Nov 10, 2019

feat: 🎸 maxMatches to limit matches and exit early #238

feat: 🎸 maxMatches to limit matches and exit early #238

Conversation

joscha commented Nov 5, 2019

What is the purpose of this pull request?

What changes did you make? (Give an overview)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrmlnc left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrmlnc Nov 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joscha commented Nov 8, 2019

mrmlnc commented Nov 8, 2019

joscha commented Nov 10, 2019

feat: 🎸 `maxMatches` to limit matches and exit early #238

feat: 🎸 `maxMatches` to limit matches and exit early #238

mrmlnc left a comment •

edited

mrmlnc Nov 8, 2019 •

edited