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

Need for notebook pre-execute handler #212427

Closed
anthonykim1 opened this issue May 10, 2024 · 9 comments
Closed

Need for notebook pre-execute handler #212427

anthonykim1 opened this issue May 10, 2024 · 9 comments
Assignees

Comments

@anthonykim1
Copy link
Contributor

anthonykim1 commented May 10, 2024

Related: #212051

With the Python REPL IW experiment, Python extension needs access to text inside user's text input box in IW in order to determine whether they have typed complete or incomplete Python command. I assume this will be part of notebook API.

This needs to handle before the already existing execute-handler since by the time execute-handler is called is too late in a way that the cell output is already attached to the UI. We need to be able to access user's command before execute handler is called.

@anthonykim1
Copy link
Contributor Author

anthonykim1 commented May 13, 2024

Additional idea/different form of API request: We could have "change event" which would get triggered every time user would type text in the text input box. This would allow Python extension to detect whether enter was typed in empty line or with text. If empty line, we can trigger submit and execute for the IW REPL.

@amunger
Copy link
Contributor

amunger commented May 13, 2024

If empty line, we can trigger submit and execute for the IW REPL.

This sounds like a reasonable thing to just implement in core, without needing any extra API or contribution from extensions

@rebornix
Copy link
Member

rebornix commented May 14, 2024

With the Python REPL IW experiment, Python extension needs access to text inside user's text input box in IW in order to determine whether they have typed complete or incomplete Python command

It's a text document so it's already available in the EH.

We need to be able to access user's command before execute handler is called

Can you elaborate why do we need to access user's command/code before execution? (what feature it is going to support)

@anthonykim1
Copy link
Contributor Author

anthonykim1 commented May 14, 2024

It's a text document so it's already available in the EH.

Right, @amunger have also made me aware of this. We discussed this yesterday but the problem about the text document "onDidChange"? event is how do we know if user is done typing and are ready to run their command.

We could watch if user has pressed enter on a blank line like @karthiknadig mentioned to me.

  • If they have pressed enter on a blank line, we could go ask Python side to tell whether that the commands that were typed are "complete" (able to execute on enter) or "incomplete" (should not execute on enter and wait for further input)

However, I'm bit concerned that relying on constantly fired onDidChange event would be pretty expensive in terms of the number of request and response coming back and forth between Typescript side and Python side when user hasn't done typing their Python command and wants to type more into the text box. Meaning we could end up watching for every non-empty line case of user typing in their characters, which seems like a waste.

The biggest concern and the reasoning behind asking for the "pre-execute" handler is wanting to make sure Python extension can receive latest information on user's Python command before actually running the command and returning the output of it for the purposes of: #212051

Can you elaborate why do we need to access user's command/code before execution? (what feature it is going to support)

This is to ensure we can execute on "enter" Once enter is made the basic keybinding for executing commands in IW textbox and smartly determine to wait instead of executing if we can figure out something is incomplete command.

@amunger I might be missing something so feel free to comment!

@rebornix
Copy link
Member

rebornix commented May 14, 2024

If they have pressed enter on a blank line, we could go ask Python side to tell whether that the commands that were typed are "complete" (able to execute on enter) or "incomplete" (should not execute on enter and wait for further input)

I think this can be a core feature. If the input is empty, enter should not trigger an execution, just like what you get in other inputs, e.g., debug console, find widget.

@rebornix
Copy link
Member

This is to ensure we can execute on "enter" Once enter is made the basic keybinding for executing commands in IW textbox and smartly determine to wait instead of executing if we can figure out something is incomplete command.

I would argue why do we prevent users from running the code they type in if they decide to? If we want to warn users about the code being "invalid/incomplete", diagnostics is probably the right way.

@anthonykim1
Copy link
Contributor Author

anthonykim1 commented May 14, 2024

I would argue why do we prevent users from running the code they type in if they decide to?

We are not necessarily preventing users from running the code, but rather should waiting just like Python interactive interpreter provided by Python itself.

I think the goal for the "native" REPL regardless of which UI shape of form that it takes, would be to make sure the behavior of the read-evaluate-print-loop to be consistent to one provided by Python itself.

For example, when I trigger Python REPL from the terminal (via Python, or Python3), and then try to start my for loop:
for i in range(100) and then press enter, the interactive interpreter will wait with the ... on the very next line because it knows that I am not done writing my for loop.

Screenshot 2024-05-14 at 12 46 24 PM

@amunger
Copy link
Contributor

amunger commented May 15, 2024

Here is my understanding of the options discussed and their drawbacks

  • Python extension would watch the input box and update a context key from the keybinding's when clause if the input box has an incomplete input
    • the behavior depends on the update being fast enough
      • e.g. if the ext host is busy, enter would execute, otherwise it would be a newline
  • Core would call a preExecute handler (peekExecution?) on the controller to let the extension veto the command
    • Not the typical way for the behavior of a command to be determined
  • Rely on static regex contributions for core to check if the line should be executed
    • Wouldn't quite match up with the behavior of a typical REPL since we probably couldn't catch every case

@anthonykim1
Copy link
Contributor Author

No longer needed as should be resolved with: microsoft/vscode-python#23442

@anthonykim1 anthonykim1 closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2024
@microsoft microsoft locked and limited conversation to collaborators Jul 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants