-
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
fix(console-reporter): Fix issue where tree logger was ignoring --silent/-s flag #5722
Conversation
cc/ @rally25rs |
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.
Hey, thanks for the patch! Can you add tests for this fix here: https://github.com/yarnpkg/yarn/blob/master/__tests__/reporters/console-reporter.js?
To add a test for I can lend a hand with that if needed. |
@BYK @rally25rs - can you take a look at those changes? |
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 good, thanks!
@@ -16,7 +16,8 @@ type MockCallback = (reporter: Reporter, opts: Object) => ?Promise<void>; | |||
export default function<T>( | |||
Reporter: Function, | |||
interceptor: Interceptor<T>, | |||
prepare?: (reporter: Reporter) => any, | |||
prepare?: ?(reporter: Reporter) => any, | |||
opts?: Object, |
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: this can be extraOpts
and then you can avoid the opts
-> newOpts
rename.
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.
Yeah, I was thinking that too. But thought it would have been preferred to keep the argument named as opts
. Let me know if you want to change though 😄
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.
Nope, already merged :D
@@ -266,3 +266,41 @@ test('close', async () => { | |||
}), | |||
).toMatchSnapshot(); | |||
}); | |||
|
|||
test('ConsoleReporter.log is silent when isSilent is true', async () => { |
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.
Drop the 'ConsoleReporter.' prefix please :)
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.
Scratch that, just realized the whole file does this.
Summary
This PR fixes #5721 where
upgrade
was not being silent when the--silent
/-s
flags were provided. It still printed the tree. 🖨🌳Test plan
Added two new test cases.
Manual CLI tests:
Before:
After: