-
Notifications
You must be signed in to change notification settings - Fork 115
Add unmanaged comms in supervisor API #9109
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
Merged
Merged
Changes from all commits
Commits
Show all changes
48 commits
Select commit
Hold shift + click to select a range
a4d365d
Implement async channel
lionel- 1ae57db
Draft unmanaged comms
lionel- 3d386bc
Clean up comm channel when disposed
lionel- 6a962cd
Make raw comm bidirectional
lionel- cb4764a
Clean up client when removed
lionel- 21726b8
Make `RawComm` a disposable
lionel- 201d67c
Add `reject()` method
lionel- e8a41e2
Reorganise files and exports
lionel- a0e2088
Split channel into tx and rx parts
lionel- 8b89219
Return boolean from `send()` to indicate whether channel was still open
lionel- cd14914
Take handlers on comm creation instead of returning channel
lionel- 73036e0
Streamline comm closing
lionel- dcefb60
Use `switch`
lionel- 67bc9e4
Use standard `target_name` parameter name
lionel- 698028e
Revert to async-iterable approach after all
lionel- bf7f359
Make comm ID public
lionel- 991cd9e
Use camelCase
lionel- 86e4bad
Reformat utils
lionel- 0981470
Implement Ark DAP as raw comm
lionel- 9730abf
Don't list unmanaged comms as clients
lionel- d90a7e7
Remove `positron.dap` from client types
lionel- 883fd68
Log `start_debug` errors
lionel- de2a82a
Rename `DapClient.ts` to `DapComm.ts`
lionel- 9621560
Streamline and document `DapComm` in supervisor API
lionel- 32d9a52
Make `handleMessage()` async
lionel- 1419051
Review comm documentation
lionel- 7390a8b
Rename `Channel` to `ReceiverChannel`
lionel- 74148bc
Add `supervisorApi()` accessor
lionel- 530b2f1
Move check inside
lionel- d55fc83
Await request result
lionel- ca2dc91
Don't close if already closed
lionel- 8743714
Add tests for raw comms
lionel- 023c2aa
Note that requests from backend to frontend are currently not possible
lionel- ff71b2e
Throw errors from `notify()` and `request()`
lionel- 5cb0d4e
Rename `Comm` to `Client`
lionel- ff60387
Add comments from pair review with Davis
lionel- 61a2dad
Don't dispose of comm in Kallichore
lionel- 2d47608
Store `RawCommImpl` instead of `RawComm`
lionel- b0b29f3
Don't expose whole DAP comm class through supervisor API
lionel- 2fbbf97
Make comm errors a tagged union
lionel- f8be357
Include `id` field in nested JSON-RPC requests
lionel- 2da8743
Use JSON-RPC `id` field if present
lionel- dca1842
Bump Ark
lionel- 23f3d80
Rename `RawComm` to `Comm`
lionel- 3772666
No longer `host` but `ip_address`
lionel- 5d3ed25
Remove `createComm()` from public interface of `DapComm`
lionel- a8f46dc
Address code review
lionel- a6abaab
Bump Ark to 0.1.210
lionel- File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (C) 2025 Posit Software, PBC. All rights reserved. | ||
* Licensed under the Elastic License 2.0. See LICENSE.txt for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
|
||
import * as vscode from 'vscode'; | ||
import { JupyterLanguageRuntimeSession, Comm } from './positron-supervisor'; | ||
import { LOGGER } from './extension'; | ||
|
||
/** | ||
* Communication channel with the bakend. | ||
* Currently only used for testing. | ||
*/ | ||
export class ArkComm implements vscode.Disposable { | ||
readonly targetName: string = 'ark'; | ||
|
||
public get comm(): Comm | undefined { | ||
return this._comm; | ||
} | ||
|
||
private _comm?: Comm; | ||
|
||
constructor( | ||
private session: JupyterLanguageRuntimeSession, | ||
) { } | ||
|
||
async createComm(): Promise<void> { | ||
this._comm = await this.session.createComm(this.targetName); | ||
LOGGER.info(`Created Ark comm with ID: ${this._comm.id}`); | ||
} | ||
|
||
async dispose(): Promise<void> { | ||
await this._comm?.dispose(); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
From review with Davis:
We might be able to make
activate()
async, thenawait
the supervisor API, then set a globalSUPERVISOR_API
. ThensupervisorApi()
no longer needs to be async, which would make it much more ergonomic: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.
Would allow us to export error classes and use
instanceof
to match them, without contagious asyncness.