Skip to content

Correctly display native REPL execution status - #23797

Merged
Anthony Kim (anthonykim1) merged 13 commits into
microsoft:mainfrom
anthonykim1:correctExecution
Jul 18, 2024
Merged

Correctly display native REPL execution status#23797
Anthony Kim (anthonykim1) merged 13 commits into
microsoft:mainfrom
anthonykim1:correctExecution

Conversation

@anthonykim1

Copy link
Copy Markdown

Resolves: #23739

@anthonykim1 Anthony Kim (anthonykim1) added feature-request Request for new features or functionality area-repl labels Jul 11, 2024
@anthonykim1 Anthony Kim (anthonykim1) added this to the July 2024 milestone Jul 11, 2024
@anthonykim1 Anthony Kim (anthonykim1) added the skip tests Updates to tests unnecessary label Jul 12, 2024
@anthonykim1
Anthony Kim (anthonykim1) marked this pull request as ready for review July 12, 2024 06:12
Comment thread src/client/repl/replController.ts Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not convinced the test for failure is the right approach.
Users can print anything to the output. that shoudln't be used as a way to determine success/failure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change REPL server message to return execution status.

@anthonykim1
Anthony Kim (anthonykim1) marked this pull request as draft July 12, 2024 17:49
Comment thread python_files/python_server.py Outdated
return True
except KeyboardInterrupt:
print(traceback.format_exc())
return False

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a different status right? Interrupted, does not really fall into pass/fail

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Screenshot 2024-07-14 at 9 35 41 PM

I think it is still fail judging from the interactive window (from Jupyter) behavior.

Comment thread src/client/repl/replController.ts Outdated
@anthonykim1
Anthony Kim (anthonykim1) marked this pull request as draft July 15, 2024 20:40
new vscode.NotebookCellOutput([vscode.NotebookCellOutputItem.text(result.output, 'text/plain')]),
]);
exec.end(false);
// TODO: Properly update via NotebookCellOutputItem.error later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you point me to the bug you filed for this on Jupyter?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure I understand the issue here, .NET, Julia, R and others are using VS COde Notebook API today, hence not sure how/why the existing API is not sufficient.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure this is the right approach, if there are failures, then you need to use error output, not text.
Not sure text output has been used here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Just added some question, response in microsoft/vscode-jupyter#15855
I did take a look at Julia repo and see they are using .error on their error output as we desired too.

Don Jayamanne (@DonJayamanne) Do you also know a way to use julia in interactive window? I cant locate some sort of dropdown/command to launch interactive window with julia kernel.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

julia-vscode/julia-vscode#2713 (comment) seems to be the most closest I can find as launching with some? drawbacks.

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

Labels

area-repl feature-request Request for new features or functionality skip tests Updates to tests unnecessary

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support correct execution status for native REPL

3 participants