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 option and CLI flag to hide command output #173

Merged
merged 7 commits into from
Oct 2, 2021
Merged

Add option and CLI flag to hide command output #173

merged 7 commits into from
Oct 2, 2021

Conversation

firoxer
Copy link
Contributor

@firoxer firoxer commented Oct 23, 2018

Hi,

I noticed #138 and thought it might be doable. Here's a proof-of-concept that does the job as proposed by @gustavohenke in the aforementioned issue.

This is not a complete PR because some edge cases might be missing and the documentation is incomplete for this part. I'll get to them if I get an OK on the POC.

Issue #25 is affected by this PR as well, as the proposed --hide flag allows for implementing --silent/-s more easily.

Thanks for creating concurrently!

@coveralls
Copy link

Pull Request Test Coverage Report for Build 325

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 99.198%

Totals Coverage Status
Change from base Build 323: 0.02%
Covered Lines: 242
Relevant Lines: 242

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 325

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 99.198%

Totals Coverage Status
Change from base Build 323: 0.02%
Covered Lines: 242
Relevant Lines: 242

💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 23, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling a41913e on firoxer:add-option-to-hide-output into fc2817a on kimmobrunfeldt:master.

Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

This PR is awesome! Thanks for taking the stab!

I'm curious about this:

This is not a complete PR because some edge cases might be missing and the documentation is incomplete for this part

What else do you think should be covered by your PR?

I only miss some functional tests, that make sure the CLI flag will work,
but I'm happy to know your input!

@firoxer
Copy link
Contributor Author

firoxer commented Nov 1, 2018

What else do you think should be covered by your PR?

I was just pointing out that I'm not confident that all use cases have been covered. :) If you feel it's complete then it's all good.

I added some functional tests and they seem to work fine. While doing that, I noticed that because --hide uses the yargs array type, it makes it necessary to use -- after the final hideable index/name when --hide is the last option because otherwise yargs won't know where stop parsing indices/names and where to start parsing commands. In other words, if the first and the third process of bin/concurrently.js 'echo 1' 'echo 2' 'echo 3' should be hidden, the command should look like bin/concurrently.js --hide 0 2 -- 'echo 1' 'echo 2' 'echo 3'. I expanded the usage notes to include this.

However, I was left wondering if this kind of behaviour is fine, because it kind of conflits with --names which works with comma-separated values instead of spaces (e.g. --names foo,bar, no following -- needed). The problem is in that by telling yargs to use array, we get automatic casting to numbers (i.e. --hide 1 foo 2 bar results in an array of [1, 'foo', 2, 'bar']) which works really well with the rest of the code. By making --hide work like --names we lose this nicety, but we gain usage consistence. Which way do you prefer?

@AntonNiklasson
Copy link

Awesome job, I would love to see this merged. However, I would expect the name of the flag to be "quiet" and not "hide". Thoughts?

@firoxer
Copy link
Contributor Author

firoxer commented Jan 13, 2019

Sure. I just went with "hide" since it was suggested in #138. I guess CLI tools usually use -q or --quiet with zero arguments so --quiet [number] would deviate from that, but on the other hand I don't think it's that big of a deal. If you want me to change it, I'll do it.

@StefanFrederiksen
Copy link

This use-case is very ideal for me, so is there anything that still needs to be done for this PR?

@firoxer
Copy link
Contributor Author

firoxer commented Jul 5, 2019

From my part I only need to know if we want to use --hide or --quiet and I'll change the code if necessary.

@StefanFrederiksen
Copy link

I think --silent is more verbose as to the intent of the flag. Otherwise --quiet since a hide flag that makes the child process not output anything doesn't seem intuitive (at least to me).

@munrocket
Copy link

munrocket commented Feb 22, 2020

Very useful PR why you not merge it @kimmobrunfeldt @gustavohenke ?

@bohdanbirdie
Copy link

@kimmobrunfeldt any chance merging this? It would be really great to have it!
Thanks!

@cben
Copy link

cben commented Jun 17, 2020

@firoxer can you resolve merge conflicts?

@firoxer
Copy link
Contributor Author

firoxer commented Jun 18, 2020

@cben Sure, done.

@munrocket
Copy link

@shilomagen
Copy link

This is an amazing feature we're waiting for :)
Do you plan to merge it soon? Thank you so much!

@gustavohenke gustavohenke self-requested a review October 2, 2021 05:52
@coveralls
Copy link

coveralls commented Oct 2, 2021

Coverage Status

Coverage increased (+0.03%) to 98.618% when pulling 863e384 on firoxer:add-option-to-hide-output into 08eda4f on open-cli-tools:master.

Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

Wow. Nearly 3 years later... I pushed some small changes, most notably not using the yargs array type.

LGTM now, let's ship it.

I feel bad that I needed the harsh COVID-19 restrictions in Sydney + a rainy long weekend for me to do this

@gustavohenke gustavohenke linked an issue Oct 2, 2021 that may be closed by this pull request
@gustavohenke gustavohenke merged commit 88b8d19 into open-cli-tools:master Oct 2, 2021
@gustavohenke
Copy link
Member

gustavohenke commented Nov 13, 2021

Published in v6.4.0!

@gustavohenke gustavohenke mentioned this pull request May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to hide command output
9 participants