Skip to content
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 command-{name|index} and !command-{name|index} condition to --success #318

Merged
merged 7 commits into from
May 15, 2022

Conversation

gustavohenke
Copy link
Member

@gustavohenke gustavohenke commented May 7, 2022

Implements 2 (or 4?) new possible conditions for --success flag:

  • command-{name|index}: command with the specified name or index must exit with 0;
  • !command-{name|index}: all but the command with the specified name or index must exit with 0.

I think that it expanding the use of ! to !first and !last could come next, but for now that's an advanced enough solution!

Closes #280.
Related: #281

@coveralls
Copy link

coveralls commented May 9, 2022

Coverage Status

Coverage increased (+0.07%) to 97.879% when pulling 7e0c855 on success-v2 into c5231ea on master.

Copy link
Collaborator

@paescuj paescuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @gustavohenke, the implementation looks good!

Basically, I like the solution with the command-{name|index} pattern. Just two thoughts from my side:

  • The command name is not unique (while the index is), there can be multiple commands with the same name. It is important to understand that in this case, the command that has finished first (!) will be considered. Should we add a note for this?
  • It can happen that the command with the specified name or index does not exist.
    In the case of !command- concurrently exits with code 0, in the case of command- concurrently exits with code 1. Should we cover this in a test case? Should we also add a note for this?

src/completion-listener.ts Outdated Show resolved Hide resolved
src/completion-listener.ts Show resolved Hide resolved
src/completion-listener.ts Outdated Show resolved Hide resolved
src/completion-listener.ts Outdated Show resolved Hide resolved
@gustavohenke gustavohenke requested a review from paescuj May 13, 2022 11:55
@gustavohenke
Copy link
Member Author

there can be multiple commands with the same name

Addressed that. Now all commands with the same name must meet the same condition.
I've also amended --success help to use the plural.

Noteworthy: input handling also accepts a target command name, but it's also only sending it to the first matching command...

It can happen that the command with the specified name or index does not exist.
In the case of !command- concurrently exits with code 0, in the case of command- concurrently exits with code 1.
Should we cover this in a test case? Should we also add a note for this?

Covered it in tests -- might be fine to get away without more docs... I'd love to be able to write more about each flag, but --help already is pretty long 😞

Comment on lines 11 to 12
* - `command-{name|index}`: only the command with the specified name or index.
* - `!command-{name|index}`: all commands but the one with the specified name or index.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* - `command-{name|index}`: only the command with the specified name or index.
* - `!command-{name|index}`: all commands but the one with the specified name or index.
* - `command-{name|index}`: only the commands with the specified name or index.
* - `!command-{name|index}`: all commands but the ones with the specified name or index.

@paescuj
Copy link
Collaborator

paescuj commented May 13, 2022

Great work, LGTM!

Addressed that. Now all commands with the same name must meet the same condition.
I've also amended --success help to use the plural.

Nice, that makes very much sense 👍

Noteworthy: input handling also accepts a target command name, but it's also only sending it to the first matching command...

Okay, might be something to address in the future...

Covered it in tests -- might be fine to get away without more docs... I'd love to be able to write more about each flag, but --help already is pretty long 😞

Fine for me, I think it's a rare case anyway 👍

Yeah, it would be handy to have a different place to to document such details!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add success condition all but first
3 participants