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 terminals in favor of jupyter_server_terminals extension #651

Merged
merged 13 commits into from
Apr 14, 2022

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Dec 30, 2021

Not ready for review yet. I'll ping reviewers here once it's ready.

This is an early preview of removing terminals from jupyter_server in favor of offering terminals as a Jupyter Server extension. This work was started by @blink1073 in jupyter-server/jupyter_server_terminals.

@Wh1isper
Copy link
Contributor

Wh1isper commented Feb 2, 2022

Just a reminder if TerminalManager should be configurable, like several KernelManager; Terminal extensionmay also need to be adjusted to support this feature. @blink1073
Thank you!

@blink1073
Copy link
Contributor

@Wh1isper, yep, that is handled here: https://github.com/jupyter-server/jupyter_server_terminals/pull/2/files#diff-686b7a6f17e88cd5881544d8ce0bfc4e4224283a68597c33551bbb886136b335R32

@blink1073
Copy link
Contributor

Kicking CI to pick up the released package

add current_activity hook for extensions

add jupyter_server_terminals as a dependency

cleanup

cleanup
@blink1073 blink1073 force-pushed the jupyter_server_terminals branch from cc2b9af to d6b6ee6 Compare April 2, 2022 19:08
@blink1073
Copy link
Contributor

We need a bit more shimming to get nbclassic downstream test to pass, and we need to add auth per jupyter-server/jupyter_server_terminals#10

@blink1073
Copy link
Contributor

Plus there are two tests that hang that are temporarily skipped

@blink1073
Copy link
Contributor

Kicking CI

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Merging #651 (2450cec) into main (64555ff) will decrease coverage by 0.54%.
The diff coverage is 48.57%.

@@            Coverage Diff             @@
##             main     #651      +/-   ##
==========================================
- Coverage   70.50%   69.96%   -0.55%     
==========================================
  Files          62       62              
  Lines        7555     7368     -187     
  Branches     1248     1223      -25     
==========================================
- Hits         5327     5155     -172     
- Misses       1840     1841       +1     
+ Partials      388      372      -16     
Impacted Files Coverage Δ
jupyter_server/terminal/__init__.py 0.00% <0.00%> (-79.17%) ⬇️
jupyter_server/terminal/api_handlers.py 0.00% <0.00%> (-100.00%) ⬇️
jupyter_server/terminal/handlers.py 0.00% <0.00%> (-75.00%) ⬇️
jupyter_server/terminal/terminalmanager.py 0.00% <0.00%> (-88.47%) ⬇️
jupyter_server/extension/manager.py 92.75% <25.00%> (-0.85%) ⬇️
jupyter_server/extension/application.py 73.09% <50.00%> (+0.69%) ⬆️
jupyter_server/serverapp.py 65.11% <50.00%> (-0.15%) ⬇️
jupyter_server/pytest_plugin.py 83.80% <70.00%> (-0.37%) ⬇️
jupyter_server/extension/handler.py 69.35% <100.00%> (+10.73%) ⬆️
jupyter_server/base/zmqhandlers.py 52.40% <0.00%> (-4.28%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64555ff...2450cec. Read the comment docs.

@blink1073
Copy link
Contributor

Looks like the last piece we need is jupyter-server/jupyter_server_terminals#15.

I'll pick this back up on Monday.

@blink1073 blink1073 closed this Apr 3, 2022
@blink1073 blink1073 reopened this Apr 3, 2022
@blink1073
Copy link
Contributor

Great success!

@Zsailer
Copy link
Member Author

Zsailer commented Apr 4, 2022

Nice work, @blink1073!

@blink1073
Copy link
Contributor

With the most recent push, we no longer have to use the awkward initialize() that is currently in jupyterlab_server and jupyter_server_terminals, but we are tolerant to their existence.

The one place we might hit a circular call stack is if the subclass initialize() has no other arguments and calls ExtensionHandlerMixin.initialize(self, name) directly.

@Zsailer Zsailer marked this pull request as ready for review April 14, 2022 16:37
@Zsailer Zsailer mentioned this pull request Apr 14, 2022
9 tasks
@blink1073 blink1073 merged commit 64bfcea into jupyter-server:main Apr 14, 2022
@Zsailer Zsailer deleted the jupyter_server_terminals branch January 16, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants