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

Make ExtensionHandler a Mixin #174

Merged

Conversation

echarles
Copy link
Member

This is a first dry attempt to support extension handlers needed by JupyterLab jupyterlab/jupyterlab_server#79

The corresponding issue is #173

This current changes allow make the jlab extensions operational, but this implementation is suboptimal as it duplicates code (with minor changes) from the base package.

@echarles
Copy link
Member Author

@Zsailer @kevin-bates As discussed during the jupyter server meeting, another option is making the ExtensionHandler a Mixin. I will give it a try in this PR.

@Zsailer
Copy link
Member

Zsailer commented Feb 4, 2020

I think it's more appropriate to make ExtensionHandler a mixin class. None of the base Jupyter Server Handler classes should have to depend on ExtensionHandler. Rather, the ExtensionHandlerMixin should extend (pardon the pun) the base server handlers by mixing in extension-specific logic.

Unfortunately, this is a backwards-incompatible change from 0.2.x. I think we need to backport this to 0.2.x, because this could cause issues for us later if other projects depend on 0.2.x first and then switch to 0.3.x.

@echarles
Copy link
Member Author

None of the base Jupyter Server Handler classes should have to depend on ExtensionHandler. Rather, the ExtensionHandlerMixin should extend (pardon the pun) the base server handlers by mixing in extension-specific logic.

@Zsailer Are you saying that ExtensionHandlerMixin should extend all base server handlers (TerminalHandlers...)? I am not sure to understand. Maybe a small asciitext class hierarchy will help to be sure what you have in mind

@echarles echarles changed the title [WIP] Add extension handlers [WIP] Make ExtensionHandler a Mixin Feb 12, 2020
@echarles
Copy link
Member Author

echarles commented Feb 12, 2020

@Zsailer @kevin-bates I have renamed that PR from [WIP] Add extension handlers to [WIP] Make ExtensionHandler a Mixin and force-pushed the needed changes for this. I am using this branch for the jupyterlab-server migration to extension handlers and test infrastructure (mock...) usage.

This works well so far for the main LabHandler and I will further work on the tests for the other handlers like WorkspacesHandler and SettingsHandler.

Please note the construct I use now and which is subject to discussion/update: class SettingsHandler(ExtensionHandlerMixin, ExtensionHandlerJinjaMixin, APIHandler):

@Zsailer
Copy link
Member

Zsailer commented Feb 12, 2020

This is looking great, @echarles!

Please note the construct I use now and which is subject to discussion/update: class SettingsHandler(ExtensionHandlerMixin, ExtensionHandlerJinjaMixin, APIHandler):

This is exactly what I had in mind. 😎

It does make the inheritance list a little long, but I think it's the most readable, transparent, and Pythonic form.

@echarles
Copy link
Member Author

@Zsailer I have progressed on the jupyterlab-server SettingsHandler using the jupyter-server test plugin and the ExtensionHandlerMixin. All good so far. We can confirm this at today server meeting and take a decision to move forward.

@echarles echarles changed the title [WIP] Make ExtensionHandler a Mixin Make ExtensionHandler a Mixin Feb 20, 2020
Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

LGTM, @echarles! Any reason not to merge this?

@Zsailer Zsailer merged commit 7cf9436 into jupyter-server:master Feb 27, 2020
Zsailer pushed a commit to Zsailer/jupyter_server that referenced this pull request Nov 18, 2022
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
…n_handlers

Make ExtensionHandler a Mixin
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.

2 participants