-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 methods to custom and buffered consoles #5514
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5514 +/- ##
==========================================
+ Coverage 61.69% 61.69% +<.01%
==========================================
Files 213 213
Lines 7077 7143 +66
Branches 4 3 -1
==========================================
+ Hits 4366 4407 +41
- Misses 2710 2735 +25
Partials 1 1
Continue to review full report at Codecov.
|
_console | ||
.getBuffer() | ||
.map(log => log.message) | ||
.join('\n'); |
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.
you can use strip-ansi
here to avoid having to use chalk inline. up to you 🙂
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.
Yes, maybe it's the way to go, but that way we don't test the bold effect.
WDYT?
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.
good point!
|
||
beforeEach(() => { | ||
_console = new CustomConsole(process.stdout, process.stderr); | ||
// override the _logToParentConsole method to be able and assert on the stdout. |
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.
You can use jest.spyOn
instead, perhaps?
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.
Yes, indeed, thanks.
info() { | ||
BufferedConsole.write(this._buffer, 'info', format.apply(null, arguments)); | ||
debug(...args: Array<mixed>) { | ||
this._log('debug', format.apply(null, arguments)); |
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.
any reason this (and the others) are still format.apply(null, arguments)
instead of format(...args)
?
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.
We can change it to format(...args)
everywhere, but then we'll need to change the flow notation...args: Array<mixed>
to ...args: Array<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.
That's fine, isn't it?
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.
If we'll decide to go with changing the signature's type notation, we'll need to also do it in the console type definition. Maybe i'm getting something wrong here, I would love to know your thoughts on this.
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.
thinking about it, doing console.log({})
is perfectly valid, so mixed
is correct
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.
That's true.
What do you say about (...args: Array<any>)
and format(...args)
?
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.
sounds good!
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!
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
👋 Hey
This PR solves issue #3756.
Summary
The addition of missing methods to
custom console
andbuffered console
.Test plan
_logToParentConsole
method.concerns
custom console
andbuffered console
.log
,warn
,info
) aren't tested, should we get full coverage there?console.table
method is missing.Let me know what can be improved, thanks!