-
Notifications
You must be signed in to change notification settings - Fork 298
Add KernelSpecManager.get_all_specs #93
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
Conversation
jupyter_client/kernelspec.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is IMO misnamed, at least I have trouble seeing this NOT as a function, which gets a helper object, but it returns a kernel spec as far as I see it. So _get_kernel_spec_for_name(name, dir)
or _make_kernel_spec(...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur with what you say, @JanSchulz. Let me update the code.
Does this also need a change in the notebook to take advantage of this new thing? [Edit: yes, it also iterates over all kernels: https://github.com/jupyter/notebook/blob/master/notebook/services/kernelspecs/handlers.py#L56] |
Noted, not sure if it'll be include in the 4.1 release but I don't see any reason as to why it shouldn't. |
jupyter_client/kernelspec.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call this get_all_specs
. That way get_
returns kernelspec objects, while find_
returns locations on disk.
And don't forget to add a docstring.
Thanks! |
@captainsafia feel free to ping with a comment when you've addressed notes, so we can review/merge; we don't get notified of new commits, only comments. Thanks again, this is great! |
Add KernelSpecManager.get_all_specs
Yep! For some strange reason, I thought i had more work to do on this before it was merge ready but if it looks good, sweet! |
I remember it now. Had to rename the |
Closes #66.
cc/ @minrk @Carreau