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

Correctly display native REPL execution status #23797

Merged
merged 13 commits into from
Jul 18, 2024

Conversation

anthonykim1
Copy link

Resolves: #23739

@anthonykim1 anthonykim1 added feature-request Request for new features or functionality area-repl labels Jul 11, 2024
@anthonykim1 anthonykim1 added this to the July 2024 milestone Jul 11, 2024
@anthonykim1 anthonykim1 self-assigned this Jul 11, 2024
@anthonykim1 anthonykim1 added the skip tests Updates to tests unnecessary label Jul 12, 2024
@anthonykim1 anthonykim1 marked this pull request as ready for review July 12, 2024 06:12
Copy link

@DonJayamanne DonJayamanne left a comment

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
Member

@karthiknadig karthiknadig left a comment

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 anthonykim1 marked this pull request as draft July 12, 2024 17:49
except KeyboardInterrupt:
print(traceback.format_exc())
return False
Copy link
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
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.

@anthonykim1 anthonykim1 marked this pull request as ready for review July 15, 2024 04:38
@anthonykim1 anthonykim1 marked this pull request as draft July 15, 2024 20:40
@anthonykim1 anthonykim1 marked this pull request as ready for review July 16, 2024 01:11
]);
exec.end(false);
// TODO: Properly update via NotebookCellOutputItem.error later.
Copy link
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
Author

Choose a reason for hiding this comment

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

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.

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
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.

@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
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.

@anthonykim1 anthonykim1 merged commit a60f228 into microsoft:main Jul 18, 2024
40 checks passed
eleanorjboyd pushed a commit to eleanorjboyd/vscode-python that referenced this pull request Jul 30, 2024
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