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

[Issue: 103] Proof point: See if notebook-http mode can be made a "plugin" #171

Closed
wants to merge 1 commit into from
Closed

[Issue: 103] Proof point: See if notebook-http mode can be made a "plugin" #171

wants to merge 1 commit into from

Conversation

nitind
Copy link
Collaborator

@nitind nitind commented Jun 16, 2016

For issue #103
(c) Copyright IBM Corp. 2016

@nitind nitind changed the title Relocates notebook-http mode and jupyter-websocket setup into separate modules. [Issue: 103] Proof point: See if notebook-http mode can be made a "plugin" Jun 16, 2016
@nitind
Copy link
Collaborator Author

nitind commented Jun 16, 2016

Replaces PR #161

@parente
Copy link
Contributor

parente commented Jun 16, 2016

It's hard to see what comments from the last PR are addressed in this new one, which aren't, and what else might have changed over all. Can you summarize?

module_name = 'kernel_gateway.jupyter_websocket'
elif module_name == 'notebook-http':
module_name = 'kernel_gateway.notebook_http'
api_module = importlib.import_module(module_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hardcoding these, how about:

try:
  api_module = importlib.import_module(module_name)
except ImportError:
  api_module = importlib.import_module('kernel_gateway.'+module_name)

It's a cost at startup only, so the initial miss for the two built-in cases isn't a huge deal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generally sounds better, but wouldn't it cause reported ImportErrors for properly missing modules to always be one that favors the 'kernel_gateway.'+ name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Good point. We'd still get the import error, but the exception log would be useless for tracking it down.

Let's keep it the way you've got it with the if-else for now for that reason.

@nitind
Copy link
Collaborator Author

nitind commented Jun 17, 2016

Rebased in #173 , with just the specifics of the personality constructors possibly in need of more guidance.

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.

None yet

2 participants