fix some import errors in 0.4

[minrk]

Fix get_template patch, avoiding import error. Question: should the patch actually be removed? I'm not sure!

I think it might have been an erroneous find/replace that changed the app name, as this has config consequences. If it's intentional, there are still some substitutions to be made, e.g. notebook_jinja_env must be nbclassic_jinja_env.

fixes #128

[echarles]

Not sure we want to rename the extension to notebook. We may rename notebookapp. to nbclassicapp

[echarles]

If we rename the extension to notebook, we will not be able to run simultaneously notebook7 and nbclassic, which is the goal.

[minrk]

The extension has been named 'notebook' for a long time. 0.4 renamed it. The rename is only partial, which is why it's unclear to me whether this was intentional, or caught in a find/replace.

[echarles]

With the goal to run both notebook7 and nbclassic at the same time, this change was intentional

[Zsailer]

Changing the main app's name to nbclassic caused #136.

[echarles]

That is a good change

[minrk]

It's not clear to me if we need this - does nbclassic still need to patch the base handler's get_template, or only the handlers it serves? Previously, it only patched the notebook's. get_template. Since these are served from this extension, maybe no patch is needed?

The notebook_jinja2_env must at least match the extension name - it will raise KeyError as it is currently, where the name is nbclassic, but the requested env is for the notebook extension (this extension's name in 0.3).

[echarles]

Is there any issue to leave the get_template patch as it is now. We can revisit that later (it is difficult for me to justify the need of not atm, would need to take more time)

Sounds logical to rename notebook_jinja2_env to nbclassic_jinja2_env Is everything still working correctly with that change?

[minrk]

Is everything still working correctly with that change?

It's unclear to me what 'correctly' should mean here, but I don't think so. Patching jupyter_server's get_template is something we haven't ever done before, so I'm not sure it's good to start doing it now. I'm not sure what problem that would solve, but I can certainly imagine it breaking things (i.e. uses of the default jinja env won't work as they are overridden).

I believe this patch was defined to ensure that handlers inheriting from notebook's JupyterHandler would load from nbclassic's jinja env, set up by jupyter-server because notebook's jinja env was never instantiated. But that's not an issue jupyter-server's JupyterHandler has, and patching the base jupyter-server JupyterHandler makes the original jinja env inaccessible to all. So if/when the two differ, I think classes inheriting from jupyter_server.base.JupyterHandler should not use the nbclassic env.

I'm not sure what exactly will happen for e.g. an extension Handler that still imports notebook.base.JupyterHandler after dealing with the notebook_shims, etc., or indeed what should happen.

[echarles]

Thx @minrk I have left 2. comments.

This fixes #128 for the base extension, but not bundler

The following diff fixes the bundler import. May be you can enrol those changes in this PR?

diff --git a/nbclassic/bundler/handlers.py b/nbclassic/bundler/handlers.py
index f72cbdce..aef909bb 100644
--- a/nbclassic/bundler/handlers.py
+++ b/nbclassic/bundler/handlers.py
@@ -7,8 +7,8 @@ from ipython_genutils.importstring import import_item
 from tornado import web, gen
 
 from jupyter_server.utils import url2path
-from nbclassic.base.handlers import IPythonHandler
-from nbclassic.services.config import ConfigManager
+from jupyter_server.base.handlers import JupyterHandler
+from jupyter_server.services.config import ConfigManager
 
 from . import tools
 
@@ -30,7 +30,7 @@ def maybe_future(obj):
         return f
 
 
-class BundlerHandler(IPythonHandler):
+class BundlerHandler(JupyterHandler):
     def initialize(self):
         """Make tools module available on the handler instance for compatibility
         with existing bundler API and ease of reference."""

[minrk]

Maybe the handlers bundled here that call get_template should indeed inherit from an nbclassic.base.BaseHandler that sets the right get_template

[echarles]

Maybe the handlers bundled here that call get_template should indeed inherit from an nbclassic.base.BaseHandler that sets the right get_template

Sounds good to me.

[echarles]

@minrk I am happy to merge this without the extension name change and cut a new release ASAP to see if it solves the issues encountered when used with JupyterHub..

I propose to discuss that specific topic at this week notebook community call. We can always open an other PR if we decide to change the extension name.

[minrk]

I looked at creating a BaseHandler, but this also appears unnecessary because of how the Handler classes inherit in nbclassic already, so having reverted the extension un-rename, I think the main fixes are in place.

It may still prove useful to solve the get_template patch when combining shims, legacy extensions, etc. I'm not sure, though.

[echarles]

Thx @minrk. I will merge as-is, cut a release and report the discussion we will have for the points you raised.

[minrk]

Testing in JupyterHub and 0.4.3 apparently now causes our tests to hang, which is a surprise! jupyterhub/jupyterhub#3977

I'll investigate further. I'd wager an issue related to Application.instance() or IOLoop.current() or some such singleton not being handled correctly, and likely only related to our test suite's peculiarity and not a real deployment. I don't quite get what the 0.4.x connection would be, though.

[minrk]

Looks like it's all on our side. Nothing to fix here.

[echarles]

Thx for the feedback @minrk and happy to see you have a fix. Hope you won't face more issue from now.

We have discussed the question of the extension name and I confirm it should stay as nbclassic for the reason I gave: we want to run simultaneously notebook v7 and nbclassic on the same jupyter-server.

[minrk]

Makes perfect sense!