-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 silent flag to allow raw output #2420
Conversation
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 makes sense and can be useful.
I think a unit test won't be necessary.
Can you add to your test plan how it behaves with other commands:
--version
install
@@ -142,6 +143,7 @@ export default class ConsoleReporter extends BaseReporter { | |||
} | |||
|
|||
_log(msg: string) { | |||
if (this.isSilent) { return; } |
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.
style nit: afaik we don't inline if blocks, could you expand it to 3 lines?
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.
Sure thing. Here you go: 001aee0
@bestander Since the goal of this feature is to enable interoperability between scripts and CLI tools, it should only be used together with the |
@rafaelrinaldi, the change touches the generic src/cli/index.js and base-reporter.js, would not it affect all the commands then? |
@bestander It would only affect the |
The change touches ConsoleReporter which is injected into config and then used in many places. |
@bestander As my PR says, this is a feature available on Let me know if either intent or implementation are not clear. |
this.format = (chalk: any); | ||
this.isSilent = !!opts.isSilent; |
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.
The intent is great, thanks for starting the PR.
The implementation confuses me because ConsoleReporter is used in other commands, not only in run
.
In the 3 changed files I don't see this setting being used for only run command.
As a matter fact, I think silent could be as useful for other commands, too.
We just need to be aware of the effects.
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.
@bestander Got it. What do you think I should do? The only file that made sense to me during implementation was console-reporter.js
.
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 think your solution makes total sense even for install and check commands.
Just add it to your test plan and check what happens if you run other commands with this flag.
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.
@bestander Alright, I will give it a shot as soon as I can. Thanks for the feedback 👍
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.
@rafaelrinaldi – I'd love to see this merged, can I help in any way?
This is a much needed feature (as demonstrated by #788) @rafaelrinaldi let me know if you want any help on this. |
@jonathandelgado Thanks for volunteering. My plan is to submit my updates this weekend so @bestander can review. |
👍 although does it also remove the measured time of the task running? that might be a feature still useful in "silent" mode |
@mhipszki My idea is to reproduce exactly what npm offers. If we decide to change, we can do it separately. |
@rafaelrinaldi makes sense! |
So I tested this on install command, as expected, all the regular logs of install were hidden but progress bar and warnings were shown. Actually I need this exact feature for a tool where Yarn's verbose install log is unnecessary, so I think we should get this feature in in some way.. However the flag should not be @rafaelrinaldi, how about |
@bestander Okay, I'm good with switching the names. Will update my PR. |
@rafaelrinaldi ping, it would be great to get this in, do you have a chance to update the PR? |
@bestander Sorry, been working like crazy lately and didn't had a chance to update. Wait until Saturday if possible, I will have some free time and can work on this 👍 |
Thanks :)
…On Tue, 21 Feb 2017 at 13:42, Rafael Rinaldi ***@***.***> wrote:
@bestander <https://github.com/bestander> Sorry, been working like crazy
lately and didn't had a chance to update. Wait until Saturday if possible,
I will have some free time and can work on this 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2420 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACBdWCykqZAp73htLwPgsKY8GxI9v2mXks5reunhgaJpZM4Le-TG>
.
|
@bestander I did most changes yesterday. I will submit a PR as soon as I get to my computer (probably tonight). |
Thanks for tackling this, @rafaelrinaldi! I run into this quite often! |
ping |
@bestander Sorry for taking a long time to tackle this, been very busy. I'm gonna have some free time at the end of the week and will be able to update this properly. |
@@ -75,6 +75,10 @@ commander.option( | |||
'--no-emoji', | |||
'disable emoji in output', | |||
); | |||
commander.option( | |||
'-s, --silent', | |||
'raw script output', |
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.
nit: silence Yarn console logs
would be closer to correct.
Technically it is not doing anything to raw output.
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.
Nevermind, I just changed the message quickly.
Thanks for rebasing!
And is it possible to make every |
@le93us It is definitely possible but I don't believe this is the expected behavior. You could perhaps solve that with a local alias. |
Summary
This PR implements a
--silent
flag (-s
for short) to Yarn's CLI in order to allow raw script output. This feature is available on npm and is very useful for when you want to pipe the output of a script as an input of another script.Test plan
Add a
foo
script to yourpackage.json
that prints a simple message:The default behavior will give you the script output followed by a header and a footer added by Yarn:
However, if you want only the raw script output you can append the
--silent
flag:$ yarn run foo --silent # or simply `-s` it works
This is my first PR to Yarn so please let me know if I need to change anything in order to make it acceptable for review. Thanks!