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.

Sign up for GitHub

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

Jump to bottom

Categorize kernels in the notebook kernel picker #135502

Merged
merged 6 commits into from
Oct 25, 2021
Merged

Categorize kernels in the notebook kernel picker #135502

merged 6 commits into from
Oct 25, 2021

Conversation

DonJayamanne
Copy link
Contributor

For microsoft/vscode-jupyter#7373 (comment)

  • Display currently selected & preferred items on top under Suggested
  • Display rest of the kernels grouped by category or extension id (if theres not category)

image

@DonJayamanne DonJayamanne requested review from jrieken, roblourens, IanMatthewHuff, rchiodo and rebornix and removed request for jrieken October 20, 2021 18:40
jrieken
jrieken requested changes Oct 21, 2021
View reviewed changes
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

Thanks so far - some nit and simplifications. Proposed API enforcement is important and I believe suggested vs suggestions too

src/vs/vscode.proposed.d.ts Show resolved Hide resolved
src/vs/vscode.proposed.d.ts Outdated Show resolved Hide resolved
src/vs/workbench/api/common/extHostNotebookKernels.ts Show resolved Hide resolved
src/vs/workbench/api/common/extHostNotebookKernels.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/notebook/browser/notebookKernelServiceImpl.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/notebook/browser/notebookKernelServiceImpl.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/notebook/browser/contrib/editorStatusBar/editorStatusBar.ts Show resolved Hide resolved
src/vs/workbench/contrib/notebook/browser/contrib/editorStatusBar/editorStatusBar.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/notebook/browser/contrib/editorStatusBar/editorStatusBar.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/notebook/browser/contrib/editorStatusBar/editorStatusBar.ts Outdated Show resolved Hide resolved
@rebornix rebornix assigned DonJayamanne and unassigned rebornix Oct 21, 2021
@jrieken
Copy link
Member

jrieken commented Oct 22, 2021

There is a slight semantic change with what suggested kernels are - I believe the outcome is that you often don't have suggestions anymore (like when no kernel is marked preferred). Unsure if that's desired or not

@DonJayamanne
Copy link
Contributor Author

I believe the outcome is that you often don't have suggestions anymore (like when no kernel is marked preferred). Unsure if that's desired or not

that's an oversight, I've fixed that now. thanks

jrieken
jrieken approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@DonJayamanne DonJayamanne merged commit 48eb908 into microsoft:main Oct 25, 2021
This was referenced Oct 25, 2021
Closed
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2021
@jrieken
Copy link
Member

jrieken commented Mar 30, 2022

@DonJayamanne @rebornix I believe we should bring this proposal to finalization

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IanMatthewHuff IanMatthewHuff IanMatthewHuff approved these changes

@jrieken jrieken jrieken approved these changes

@rchiodo rchiodo rchiodo approved these changes

@rebornix rebornix Awaiting requested review from rebornix

@roblourens roblourens Awaiting requested review from roblourens

Assignees

@DonJayamanne DonJayamanne

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@DonJayamanne @jrieken @roblourens @IanMatthewHuff @rchiodo @rebornix