Runner service implementation feature parity #226
Conversation
mxsdev
force-pushed
the
runner-service-fixes
branch
from
64dc36d
to
4c5da8e
Compare
|
Should be mostly done, basically waiting on #155. I would like to do more testing on the user prompting for env variables, both with and without the grpc runner, since this got changed a bit on both ends. |
There was a problem hiding this comment.
Left some comments in the code. From a functional point of view, it appears that "export-only" cells have some unindented behavior.
Please see this clip: https://i.imgur.com/jqYi1yH.mp4
We could rebase and merge this PR and move the remaining issue to a new one. Up to you @mxsdev.
| const MIME_TYPES_WITH_CUSTOM_RENDERERS = ['text/plain'] | ||
|
|
||
| export async function executeRunner( | ||
| context: ExtensionContext, |
There was a problem hiding this comment.
Side note: one of the reasons why I think this the function-abstraction of Executors won't suffice long-term. Not part of this PR, though.
| /** | ||
| * evaluate expression | ||
| */ | ||
| const expression = placeHolder.slice(2, -1) | ||
| const expressionProcess = cp.spawn(expression, { | ||
| const expressionProcess = cp.spawn(value, { |
There was a problem hiding this comment.
Shouldn't this use the Runner APIs versus spawning child processes? Consider addressing in follow up issue/PR.
There was a problem hiding this comment.
The runner executor uses getCommandExportExtractMatches directly - it does not call retrieveShellCommand.
There was a problem hiding this comment.
great 👌 , thanks for clarifying!
| @@ -106,8 +111,8 @@ export function getKey(runningCell: vscode.TextDocument): keyof typeof executor | |||
| * treat cells like like a series of individual commands | |||
| * which need to be executed in sequence | |||
| */ | |||
| export function getCmdShellSeq(cellText: string, os: string): string { | |||
| const trimmed = cellText | |||
| export function getCmdSeq(cellText: string): string[] { | |||
There was a problem hiding this comment.
Once the TUI is using the Runner to execute commands we should consider GRPC exposing APIs to prep an input as a sequence of commands so both Ext/TUI plausible work the same way. Let's make a ticket for this if we haven't already.
|
Tested with the main example readme and every script worked for me. Could you test against really quick with the latest changes? Thanks! |
Confirmed. Great job 👏 Another thing I noticed is that |
mxsdev
force-pushed
the
runner-service-fixes
branch
from
8cd9a29
to
8bc708c
Compare
|
I was able to fix tests, but for the following line in vi.mock('../../../src/extension/utils', () => ({
replaceOutput: vi.fn(),
}))I was not able to use In the meantime I just mocked the method, but this is less than ideal since there's no need to mock it. Lmk if you think this is worth an issue @sourishkrout. Anyways, this is still good to merge! 🎉 |
background=trueas background tasksNeeds changes in runme service:
examples/README.md) with notebook-native png renderer crash-hangs #143examples/README.md) does not work#144#155