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

Remove console log usage #10341

Merged
merged 7 commits into from
Jun 7, 2022
Merged

Remove console log usage #10341

merged 7 commits into from
Jun 7, 2022

Conversation

rchiodo
Copy link
Contributor

@rchiodo rchiodo commented Jun 6, 2022

Fixes #10202

Related to the PR I have in the ipywidget npm module, this removes uses of console.log in our renderers. Mostly it was widget stuff.
microsoft/vscode-jupyter-ipywidgets#3

@rchiodo rchiodo requested a review from a team as a code owner June 6, 2022 19:39
@@ -106,7 +106,7 @@ export class IPyWidgetMessageDispatcher implements IIPyWidgetMessageDispatcher {
public receiveMessage(message: IPyWidgetMessage): void {
switch (message.message) {
case IPyWidgetMessages.IPyWidgets_logMessage:
traceInfoIfCI(`Widget Message: ${message.payload}`);
traceInfo(`Widget Message: ${message.payload}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this traceInfo because after these two changes, this is really only stuff that we actually want to show to the user (or us when request logs).

@@ -393,9 +390,6 @@ class ProxyKernel implements IMessageHandler, Kernel.IKernelConnection {
}
private messageHookInterceptor(msg: KernelMessage.IIOPubMessage): boolean | PromiseLike<boolean> {
try {
logMessageOnlyOnCI(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured if we needed these later, we can add a new IPYWidgets_LogMessageIfCI message. Then on the other side it would only log these if running on CI.

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2022

Codecov Report

Merging #10341 (ab38fb5) into main (0700879) will decrease coverage by 0%.
The diff coverage is n/a.

❗ Current head ab38fb5 differs from pull request most recent head 2104b38. Consider uploading reports for the commit 2104b38 to get more accurate results

@@          Coverage Diff          @@
##           main   #10341   +/-   ##
=====================================
- Coverage    64%      64%   -1%     
=====================================
  Files       212      212           
  Lines      9557     9557           
  Branches   1527     1527           
=====================================
- Hits       6148     6145    -3     
- Misses     2932     2934    +2     
- Partials    477      478    +1     
Impacted Files Coverage Δ
src/platform/common/platform/fileSystem.ts 81% <0%> (-5%) ⬇️

@rchiodo rchiodo merged commit 0b3ede8 into main Jun 7, 2022
@rchiodo rchiodo deleted the rchiodo/remove_console_log branch June 7, 2022 16:11
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.

Prefer an output channel over console.log
4 participants