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

Prevent first Python command being lost #22902

Merged
merged 34 commits into from Feb 16, 2024

Conversation

anthonykim1
Copy link

@anthonykim1 anthonykim1 commented Feb 12, 2024

Fixes: #22673
Fixes: #22545
Fixes: #22691

Making best effort to address issue where very first command sent to REPL via Terminal gets ignored, or gets pasted both in Terminal and in REPL.

With the fix, we observe whether Python REPL is launched in Terminal via VS Code's onDidWriteTerminalData and send the command, or wait three seconds as a fallback mechanism.

These two combined together will significantly reduce or resolve all-together the chance of very first command being swollen up or gets pasted twice in Terminal and REPL previously where it did not have context of whether Python REPL instance have started inside the Terminal or not.

@anthonykim1 anthonykim1 self-assigned this Feb 12, 2024
@anthonykim1 anthonykim1 added area-repl bug Issue identified by VS Code Team member as probable bug labels Feb 12, 2024
@anthonykim1 anthonykim1 added this to the February 2024 milestone Feb 12, 2024
@karthiknadig karthiknadig changed the title Prevent first Python command being swollen Prevent first Python command being lost Feb 14, 2024
@anthonykim1 anthonykim1 marked this pull request as ready for review February 16, 2024 09:19
new Promise<boolean>((resolve) => {
let count = 0;
const terminalDataTimeout = setTimeout(() => {
resolve(true); // Fall back for test case scenarios.
Copy link
Member

@hediet hediet Feb 16, 2024

Choose a reason for hiding this comment

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

Line 71 seems to do this already.
Generally, this code seems to be quite difficult to read (a for loop in an event listener in a promise that is returned by a promise), I suggest to reduce nesting a bit.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the feedback!! @hediet.
It's little tricky right now, since I wanted a promise that always returned true, maximum three second or faster if we see that Python REPL has launched before.
I definitely thought about removing the second timeout, but in testing scenario, I would get promise rejection since it could not read the ">>>" from TerminalData like how it would normally read from user's terminal when we launch Python REPL from shell of their choice.

Copy link
Member

Choose a reason for hiding this comment

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

but in testing scenario, I would get promise rejection

I don't understand why this would happen. Are you sure? race ignores future rejections.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I found it pretty odd too.. Perhaps its because during testing, we don't have "mocking" of Terminal write data containing the ">>>" that I check for to see if Python REPL has launched?? hence maybe thats why it may be giving rejection before we pass with the 3 second condition.

@anthonykim1 anthonykim1 merged commit e53651d into microsoft:main Feb 16, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-repl bug Issue identified by VS Code Team member as probable bug
Projects
None yet
2 participants