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

Update MappingKM.restart_kernel to accept now kwarg #404

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

vidartf
Copy link
Member

@vidartf vidartf commented Feb 5, 2021

This should bring it in line with the signature of the base class (added in jupyter_client 4.0.0).

Fixes #402.

This should bring it in line with the signature of the base class (added in jupyter_client 4.0.0).
@kevin-bates
Copy link
Member

It occurred to me that this could theoretically break subclasses of MKM that override restart_kernel(), but that would require it be called with an explicit now parameter. Since this method is only called from the handler and the handler lets now be conveyed via its default, this is not a practical problem.

While we're at it, would you mind removing the unused **kwargs parameter from GatewayManager's override of this method? I'll be responsible for any issues that might cause, although the same argument holds there, only the handler should be calling that method (when --gateway-url is in play). If you'd rather not, no worries.

@blink1073 blink1073 added this to the 1.6 milestone Mar 23, 2021
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Thanks @vidartf - sorry about the delay!

@blink1073 blink1073 merged commit 6cad9ee into jupyter-server:master Mar 24, 2021
@vidartf vidartf deleted the km-re-now branch March 25, 2021 13:51
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
Update MappingKM.restart_kernel to accept now kwarg
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.

MappingKernelManager.restart_kernel should pass through "now" keyword
3 participants