Don't pass None kernels to logging configurable in Comm

[bollwyvl]

References

• fixes Can't update children trait after widget is displayed. #1060

Changes

• doesn't pass kernel=None values to LoggingConfigurable.__init__

[maartenbreddels]

Let me add a test for this, so we can pour this in concrete.

[maartenbreddels]

From aa0693490a00f8c3ac13f64d92d45012de03c04f Mon Sep 17 00:00:00 2001
From: "Maarten A. Breddels" <maartenbreddels@gmail.com>
Date: Tue, 20 Dec 2022 15:39:04 +0100
Subject: [PATCH] test: make sure comms have the correct kernel

---
 ipykernel/tests/test_comm.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ipykernel/tests/test_comm.py b/ipykernel/tests/test_comm.py
index 405841f..44a30a5 100644
--- a/ipykernel/tests/test_comm.py
+++ b/ipykernel/tests/test_comm.py
@@ -2,6 +2,7 @@ import unittest.mock
 
 from ipykernel.comm import Comm, CommManager
 from ipykernel.ipkernel import IPythonKernel
+from ipykernel.kernelbase import Kernel
 
 
 def test_comm(kernel):
@@ -11,6 +12,8 @@ def test_comm(kernel):
     c = Comm(kernel=kernel, target_name="bar")
     msgs = []
 
+    assert kernel is c.kernel
+
     def on_close(msg):
         msgs.append(msg)
 
@@ -56,6 +59,11 @@ def test_comm_manager(kernel):
         manager.register_comm(comm)
         assert publish_msg.call_count == 1
 
+    # make sure that when we don't pass a kernel, the 'default' kernel is taken
+    Kernel._instance = kernel
+    assert comm.kernel is kernel
+    Kernel.clear_instance()
+
     assert manager.get_comm(comm.comm_id) == comm
     assert manager.get_comm('foo') is None
 
-- 
2.37.1 (Apple Git-137.1)

[maartenbreddels]

Btw, I checked, and indeed the test fails on the current main (2c66b2d)

[bollwyvl]

patched! 🧦

[maartenbreddels]

sorry for the lint issue :)

[bollwyvl]

is there a bot command to add a label... or can someone? It's annoying to get the ❌ immediately, makes the browser tab status icon worthless.

[blink1073]

We'd have to add config similar to https://github.com/jupyterlab/jupyterlab/blob/master/.meeseeksdev.yml to enable that.

[blink1073]

Thanks @bollwyvl and @maartenbreddels!

[bollwyvl]

thanks for the assist