-
-
Notifications
You must be signed in to change notification settings - Fork 236
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
Add new --matrix option to multiply commands #526
base: main
Are you sure you want to change the base?
Conversation
ecdae50
to
5af7c99
Compare
5af7c99
to
dfc3c5f
Compare
dfc3c5f
to
f656f5d
Compare
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.
Thanks for taking this up!
I made several suggestions and asked a few questions in this pass.
I'll update the docs to mention matrices as well in a follow-up, unless you feel like doing it while keeping it on brand?
* Replace placeholders with new commands for each combination of matrices. | ||
*/ | ||
export class ExpandMatrices implements CommandParser { | ||
private _bindings: string[][]; |
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.
I assume the _
is to signal that the field is private - if there isn't another field with the same name (such as a getter), let's not add that prefix.
It can also be made readonly
:
private _bindings: string[][]; | |
private readonly bindings: string[][]; |
private _bindings: string[][]; | ||
|
||
constructor(private readonly matrices: readonly string[][]) { | ||
this.matrices = matrices; |
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.
This line isn't necessary, as the private
in the constructor param list does the same job.
this.matrices = matrices; |
export class ExpandMatrices implements CommandParser { | ||
private _bindings: string[][]; | ||
|
||
constructor(private readonly matrices: readonly string[][]) { |
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.
Doesn't seem like it's read outside of the constructor, so for now let's not make it a field
constructor(private readonly matrices: readonly string[][]) { | |
constructor(matrices: readonly string[][]) { |
let index = 0; | ||
if (placeholderTarget && !isNaN(placeholderTarget)) { |
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.
Should an error be thrown if the placeholder isn't a number?
Might convert a "wtf is concurrently doing" to a "oh, my bad, I got it wrong" 😄
@@ -103,6 +103,11 @@ export type ConcurrentlyOptions = Omit<BaseConcurrentlyOptions, 'abortSignal' | | |||
* If not defined, no argument replacing will happen. | |||
*/ | |||
additionalArguments?: string[]; | |||
|
|||
/** | |||
* This command should be run multiple times, for each of the provided matrices. |
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.
Can you please update this comment? It looks like it's assuming some context from elsewhere.
@@ -179,6 +185,10 @@ export function concurrently( | |||
new ExpandWildcard(), | |||
]; | |||
|
|||
if (options.matrices?.length) { |
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 matrices have preference over passthrough arguments using the same {number}
syntax. Was it intended?
It needs to be called out in the help and in the docs.
Closes #517
Sample runs:
Single dimension:
Multiple dimensions:
With environment variables: