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

Add back ipythonhandler for backwards compatibility for notebook extensions on nbclassic #868

Closed

Conversation

echarles
Copy link
Member

Add back IPythonHandler for backwards compatibility for notebook extensions on nbclassic

This is related to jupyter/nbclassic#113

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #868 (015b7dd) into 1.x (25b8011) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              1.x     #868      +/-   ##
==========================================
+ Coverage   70.35%   70.42%   +0.06%     
==========================================
  Files          62       62              
  Lines        7603     7654      +51     
  Branches     1255     1270      +15     
==========================================
+ Hits         5349     5390      +41     
- Misses       1873     1878       +5     
- Partials      381      386       +5     
Impacted Files Coverage Δ
jupyter_server/base/handlers.py 65.46% <100.00%> (+0.13%) ⬆️
jupyter_server/services/contents/filemanager.py 72.20% <0.00%> (-0.59%) ⬇️
jupyter_server/serverapp.py 65.42% <0.00%> (-0.20%) ⬇️
jupyter_server/services/contents/handlers.py 86.55% <0.00%> (+1.09%) ⬆️
jupyter_server/services/kernels/kernelmanager.py 80.58% <0.00%> (+1.94%) ⬆️

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 25b8011...015b7dd. Read the comment docs.

@echarles
Copy link
Member Author

I see a few CI errors. Is this expected on 1.x branch?

@blink1073
Copy link
Contributor

If you merge the current branch it will fix the check-release. If you run pre-commit run --all files it will fix the lint errors. I'll backport #865 for the docs part.

@echarles echarles force-pushed the feat/ipythonhandler branch from 015b7dd to caa64a0 Compare June 15, 2022 13:10
@blink1073
Copy link
Contributor

You merged with master instead of 1.x

@echarles
Copy link
Member Author

Aargh, and I have even written it :)... Let me undo...

@echarles echarles force-pushed the feat/ipythonhandler branch from d2f5f4f to 8e496bc Compare June 15, 2022 13:20
@blink1073
Copy link
Contributor

Ah, I hadn't merged #871 yet, can you please merge from 1.x again?

@echarles echarles force-pushed the feat/ipythonhandler branch from 8e496bc to 05eb048 Compare June 15, 2022 13:30
@echarles
Copy link
Member Author

I have rebased on latest 1.x. There are still 4 failing jobs which are imho unrelated.

@echarles echarles marked this pull request as draft June 15, 2022 15:40
@echarles
Copy link
Member Author

Moving to draft as we are discussing this in the notebook meeting.

@blink1073
Copy link
Contributor

Okay, the last remaining error is due to a beta release of tornado. We'll have to handle this in main and then backport.

@kevin-bates
Copy link
Member

I was wondering if we need to worry about future enhancements made to JupyterHandler that we may not (?) want to support (or can support) in subclasses of the "classic" IPythonHandler?

If that's something to consider, we may want to either swap the two (i.e., rename JupyterHandler to IPythonHandler and let JupyterHandler be the alias for now until new changes occur), or, better yet, rename JupyterHandler to some kind of "base handler" and let both JupyterHandler and IPythonHandler be aliases. With the latter approach, using instanceof on a handler asking about either JupyterHandler or IPythonHandler would then be definitive since the two are siblings.

@echarles
Copy link
Member Author

Thx for jumping here @kevin-bates. We have been talking about this PR during today notebook meeting and other ideas have been setup like having the IPythonHandler being added in notebook or nbclassic. These ideas still need to percolate and land softly.

We are still defining who is shimming what. It sounded to me that jupyter-server was the perfect place to host those base handlers and ensure the backwards and forwards compatibilities but have not raised consensus on that so far.

If that's something to consider, we may want to either swap the two (i.e., rename JupyterHandler to IPythonHandler and let JupyterHandler be the alias for now until new changes occur), or, better yet, rename JupyterHandler to some kind of "base handler" and let both JupyterHandler and IPythonHandler be aliases. With the latter approach, using instanceof on a handler asking about either JupyterHandler or IPythonHandler would then be definitive since the two are siblings.

Sounds good to me. We could review/confirm also our current handler hierarchy at that occasion, but this will mainly depend on the outcome of the current discussion.

@Zsailer
Copy link
Member

Zsailer commented Feb 16, 2023

Hey @echarles, we're triaging PRs in the Jupyter Server meeting today. Because there hasn't been activity on this PR in a little while, I'm going to close it. Thanks for your work here and feel free to re-open anytime if this is in-progress.

@Zsailer Zsailer closed this Feb 16, 2023
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.

5 participants