-
-
Notifications
You must be signed in to change notification settings - Fork 368
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
Return Comm rather than BaseComm from create_comm #1091
base: main
Are you sure you want to change the base?
Conversation
It looks like the failure is in
My intuition doesn't have much to say about if this is related. |
It seems to be an intermittent failure. I saw it in #1089, but then it passed on the merge commit. I just kicked it. |
@@ -45,7 +45,7 @@ | |||
|
|||
def _create_comm(*args, **kwargs): | |||
"""Create a new Comm.""" | |||
return BaseComm(*args, **kwargs) |
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.
Ah, I misunderstood what you intended to change. We wanted to explicitly return BaseComm
here because it is more efficient. I think the right answer is for ipywidgets
to relax its expected trait.
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, BaseComm is 'the new thing', Comm is for backwards compatibility with ipywidgets.
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 see, but since ipywidgets
doesn't accept BaseComm
should that be the submitted change? I guess I'm confused because @blink1073's suggestion seemed straightforward, but now I don't understand how to interpret @maartenbreddels's reply.
This replaces
BaseComm
increate_comm
withComm
, which should match what is expected byipywidgets
for validation of thecomm
trait in subclasses ofipywidgets.Widget
.Fixes #1090 .