When a number is passed as concurrent parameter, execute the tasks in… #56
Conversation
|
Hi, is there anything wrong with the PR? |
|
Hi @alexpsi. No sorry, didn't find the time to look into it yet. Will try to have a look this week, quite busy lately. |
|
Thanks, no need to hurry :) |
|
Any update on this? I have to reimplement a lot of code here in several places. Would be nice to have this implemented in the core |
index.js
Outdated
| this._checkAll(context); | ||
| return runTask(task, context, errors); | ||
| })); | ||
| }), Promise.resolve()); |
There was a problem hiding this comment.
Might be worth using something like p-map to simplify the code here.
There was a problem hiding this comment.
Yap, did not know of p-map, i will try it out.
alexpsi
force-pushed
the
concurrent_limit
branch
from
f637521 to
f2b0809
Compare
|
@sindresorhus @SamVerschueren I refactored it with p-map, nice thing is that it does not have to wait for the chunk to finish anymore, so it is even better |
index.js
Outdated
| tasks = pMap(this._tasks, task => { | ||
| this._checkAll(context); | ||
| return runTask(task, context, errors); | ||
| }, {concurrency: this._options.concurrent}); | ||
| } else { | ||
| tasks = this._tasks.reduce((promise, task) => promise.then(() => { |
There was a problem hiding this comment.
Instead of this, we could just set {concurrency: 1} in the above implementation. p-map would then run in serial.
index.js
Outdated
| @@ -82,6 +83,11 @@ class Listr { | |||
| let tasks; | |||
| if (this._options.concurrent === true) { | |||
| tasks = Promise.all(this._tasks.map(task => runTask(task, context, errors))); | |||
There was a problem hiding this comment.
And this could just use the pMap() method with concurrency Infinity.
index.js
Outdated
| @@ -82,6 +83,11 @@ class Listr { | |||
| let tasks; | |||
| if (this._options.concurrent === true) { | |||
| tasks = Promise.all(this._tasks.map(task => runTask(task, context, errors))); | |||
| } else if (typeof this._options.concurrent === 'number' && this._options.concurrent > 0) { | |||
| tasks = pMap(this._tasks, task => { | |||
| this._checkAll(context); | |||
There was a problem hiding this comment.
Why is this called here, but not when this._options.concurrent === true?
There was a problem hiding this comment.
@sindresorhus Ahhh sorry, last commit was a bad paste. Next one sets it straight... :)
There was a problem hiding this comment.
Cool, this is pretty clean with p-map (thanks @sindresorhus :)). And thank you for the PR! Sorry for the late reply though.
Could you also change the docs for the concurrent option?
index.js
Outdated
| if (this._options.concurrent === true) { | ||
| tasks = Promise.all(this._tasks.map(task => runTask(task, context, errors))); | ||
| let concurrency; | ||
| if (!this._options.concurrent) { |
There was a problem hiding this comment.
I would do it the other way around
if (this._options.concurrent === true) {
concurrency = Infinity;
} else if (...) {
} else {
concurrency = 1;
}There was a problem hiding this comment.
Also wondering, wouldn't it be cleaner if we did all of this in the constructor instead? And thus set this._options.concurrent to the correct value for p-map.
index.js
Outdated
|
|
||
| return runTask(task, context, errors); | ||
| }), Promise.resolve()); | ||
| concurrency = 'Infinity'; |
There was a problem hiding this comment.
Infinity, not 'Infinity' :)
test/concurrent.js
Outdated
| renderer: SimpleRenderer, | ||
| concurrent: 2 | ||
| }); | ||
| testOutput(t, [ |
test/concurrent.js
Outdated
| 'P4 [completed]', | ||
| 'done' | ||
| ]); | ||
| await list.run(); |
index.js
Outdated
| } else if (typeof this._options.concurrent === 'number') { | ||
| this.concurrency = this._options.concurrent; | ||
| } else { | ||
| this.concurrency = 1; |
There was a problem hiding this comment.
Set this.concurrency = 1 at line 35 and remove this one
readme.md
Outdated
| Default: `false` | ||
|
|
||
| Set to `true` if you want tasks to run concurrently. | ||
| Set to `true` if you want to run tasks in parallel, set to a number to control |
There was a problem hiding this comment.
Darn, missed this one :).
Set to
trueif you want to run the tasks in parallel. Set to a number to control the concurrency.
One line, no breaks.
|
@SamVerschueren Thanks!!! |
|
Thank you for all the work (and for your patience). Releasing it as we speak ( |
… a chunked parallel manner.