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

Move the hasRunningNotebookSession context out of the notebook session service #5688

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

seeM
Copy link
Contributor

@seeM seeM commented Dec 10, 2024

Another step toward removing the notebook session service entirely (#2671).

QA Notes

This shouldn't change any behavior, although I did fix some edge cases where the context was not set correctly.

Opening a notebook, restarting it, closing and reopening it should all work. The restart button should be deactivated when the session is exited.

@seeM seeM requested a review from nstrayer December 10, 2024 17:36
Copy link
Contributor

@nstrayer nstrayer left a comment

Choose a reason for hiding this comment

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

This is nice.
Tested and everything works as expected. Looking forward to the final removal of the notebook session service!

@@ -75,7 +75,7 @@ const extensions = [
{
label: 'positron-notebook-controllers',
workspaceFolder: 'extensions/positron-notebook-controllers/test-workspace',
mocha: { timeout: 60_000 }
mocha: { timeout: 5_000 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's actually helpful to have the tests fail when something hangs after 5 seconds. This extension's tests aren't in CI atm as well.


export const log = vscode.window.createOutputChannel('Positron Notebook Controllers', { log: true });

const _onDidSetHasRunningNotebookSessionContext = new vscode.EventEmitter<boolean>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for testing purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

@seeM seeM merged commit 70ed8a3 into main Dec 11, 2024
5 checks passed
@seeM seeM deleted the 2671-positron-has-running-notebook-context branch December 11, 2024 18:36
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants