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

Allow execute on enter and Intellisense for native REPL with notebook UI #23442

Merged
merged 33 commits into from
May 24, 2024

Conversation

anthonykim1
Copy link

@anthonykim1 anthonykim1 commented May 16, 2024

"Smartly" allow execute on enter for the #23235 experiment.
User should be able to execute when they press enter on text input box of interactive window trigger from Python extension, whereas we would "wait" and allow insertion of new line after detecting user's Python command is not complete.

When the user finally types enter again on a blank line, we should just proceed to execute whatever code, regardless of whether it is complete/valid or not to replicate Python's original interactive REPL behavior.

Basically creating Python command and registering that for keybinding of 'Enter'.
This would conditionally call interactive.execute which would then eventually call our execute handler contributed from Python n extension's REPL controller, or go ahead and insert,pass in Enter to the text input box to allow user to type "complete" code.

This PR only intends to implement/add changes regarding execute on enter logic, adding Intellisense support, and also adding things into disposables so they can be properly disposed. Trying to also add setting to allow toggling on/off to send Python command to Terminal or IW REPL if the user is in experiment.

Handling of interrupt for windows should be on separate PR.
Test will be added later as separate PR.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/client/repl/replCommands.ts Outdated Show resolved Hide resolved
@anthonykim1 anthonykim1 self-assigned this May 17, 2024
@karthiknadig
Copy link
Member

Screenshot 2024-05-20 at 11 47 46 AM

This looks like the package.nls.json file is missing an entry.

python_files/ctrlc.py Outdated Show resolved Hide resolved
@anthonykim1 anthonykim1 added the feature-request Request for new features or functionality label May 20, 2024
@anthonykim1 anthonykim1 changed the title Allow execute on enter for IW REPL Allow execute on enter for IW REPL and Intellisense May 21, 2024
@anthonykim1 anthonykim1 added the skip package*.json package.json and package-lock.json don't both need updating label May 21, 2024
@anthonykim1 anthonykim1 changed the title Allow execute on enter for IW REPL and Intellisense Allow execute on enter and Intellisense for IW REPL May 21, 2024
@anthonykim1 anthonykim1 added the skip tests Updates to tests unnecessary label May 21, 2024
package.json Outdated
{
"command": "python.execInREPLEnter",
"key": "enter",
"when": "activeEditor == 'workbench.editor.interactive'"
Copy link
Author

Choose a reason for hiding this comment

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

should I also be adding !config.interactiveWindow.executeWithShiftEnter
here in the when clause? @amunger

Copy link

Choose a reason for hiding this comment

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

yes please

Copy link
Author

Choose a reason for hiding this comment

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

For some reason, adding this when clause in makes the whole command get ignored when user presses enter. (I have not personally set up any shift enter keybinding override on my machine).

Copy link
Author

@anthonykim1 anthonykim1 May 22, 2024

Choose a reason for hiding this comment

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

Found why it is not working..(As in figured out why after adding !config.interactiveWindow.executeWithShiftEnter to when clause would not let user execute with enter by default. Basically user has to have explicit "interactiveWindow.executeWithShiftEnter": false, in their settings.json.

Value for "interactiveWindow.executeWithShiftEnter" seems to be true by default, but Python extension would ideally want the default to be false. Should Python extension somehow override the value of this?

Copy link

Choose a reason for hiding this comment

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

I was planning to switch the default, but I didn't get around to putting in the migration logic for jupyter IW users. So you should be able to leave it as is for now (and manually set the setting), and I'll still try to make the switch

Copy link
Author

@anthonykim1 anthonykim1 May 22, 2024

Choose a reason for hiding this comment

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

@amunger When you say "leave it as is for now" and update the setting, do you still want the !config.interactiveWindow.executeWithShiftEnter clauses in the "when" condition, and also have Python extension manually set the "interactiveWindow.executeWithShiftEnter": false, for users?
OR
just exclude using !config.interactiveWindow.executeWithShiftEnter in when clause

Copy link

Choose a reason for hiding this comment

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

We still need the when clause here, otherwise users who want to use shift+enter to execute will not be able to use enter for a newline.
By manually, I meant users will individually need to change that setting until the default is switched over, don't change anyone's configuration since we were planning on doing that in the other direction for jupyter IW users.

Copy link
Author

Choose a reason for hiding this comment

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

got it, it should be resolved with: 910783b

@anthonykim1 anthonykim1 marked this pull request as ready for review May 21, 2024 22:12
@karthiknadig karthiknadig reopened this May 22, 2024
package.nls.json Outdated Show resolved Hide resolved
package.nls.json Outdated Show resolved Hide resolved
anthonykim1 and others added 2 commits May 22, 2024 16:46
Co-authored-by: Courtney Webster <60238438+cwebster-99@users.noreply.github.com>
@@ -628,6 +628,12 @@
"scope": "resource",
"type": "boolean"
},
"python.REPL.sendToNativeREPL": {
Copy link
Member

Choose a reason for hiding this comment

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

This does not have the experimental tag?

Copy link
Author

@anthonykim1 anthonykim1 May 23, 2024

Choose a reason for hiding this comment

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

Right, this is the settings which the value is ultimately overridden by the experiment value "pythonRunREPL".
Initially thought everything was just going to be experiment, but got convoluted with going the settings based experiment route

Following:

We also should get rid of the experiment item that we added for this, as the setting itself can be controlled via experiment by using the settings tag.

I think I would need to replace of existing "pythonRunREPL" from just experiment to settings (basically replace the setting tag from sendToNativeREPL to pythonRunREPL ). Would that allow us to leave the control tower as is? @cwebster-99

Copy link
Member

@karthiknadig karthiknadig May 23, 2024

Choose a reason for hiding this comment

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

I think it would be the fully require name python.REPL.sendToNativeREPL.

package.json Outdated Show resolved Hide resolved
package.nls.json Outdated Show resolved Hide resolved
src/client/extensionActivation.ts Outdated Show resolved Hide resolved
src/client/repl/pythonServer.ts Outdated Show resolved Hide resolved
src/client/repl/pythonServer.ts Outdated Show resolved Hide resolved
src/client/repl/replCommands.ts Outdated Show resolved Hide resolved
src/client/repl/replCommands.ts Outdated Show resolved Hide resolved
src/client/repl/replCommands.ts Outdated Show resolved Hide resolved
src/client/repl/replCommands.ts Outdated Show resolved Hide resolved
src/client/repl/replCommands.ts Outdated Show resolved Hide resolved
@karthiknadig
Copy link
Member

We also should get rid of the experiment item that we added for this, as the setting itself can be controlled via experiment by using the settings tag.

anthonykim1 and others added 2 commits May 22, 2024 18:55
Co-authored-by: Karthik Nadig <kanadig@microsoft.com>
Co-authored-by: Karthik Nadig <kanadig@microsoft.com>
@anthonykim1 anthonykim1 marked this pull request as draft May 23, 2024 02:04
@anthonykim1 anthonykim1 marked this pull request as ready for review May 23, 2024 20:35
@anthonykim1
Copy link
Author

I think I addressed all the requested changes. I will put up additional PR to better refactor this.

@anthonykim1 anthonykim1 changed the title Allow execute on enter and Intellisense for IW REPL Allow execute on enter and Intellisense for native REPL with IW UI May 23, 2024
@anthonykim1 anthonykim1 changed the title Allow execute on enter and Intellisense for native REPL with IW UI Allow execute on enter and Intellisense for native REPL with notebook UI May 23, 2024
}
}),
);
}

export async function registerReplExecuteOnShiftEnter(): Promise<void> {
commands.registerCommand(Commands.Exec_In_REPL_Shift_Enter, async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This needs a disposable, similar to other commands registered.

Copy link
Author

@anthonykim1 anthonykim1 May 23, 2024

Choose a reason for hiding this comment

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

Should be good with: e270705

@anthonykim1 anthonykim1 merged commit 75cc52b into microsoft:main May 24, 2024
42 checks passed
DonJayamanne pushed a commit that referenced this pull request Jun 24, 2024
… UI (#23442)

"Smartly" allow execute on enter for the
#23235 experiment.
User should be able to execute when they press enter on text input box
of interactive window trigger from Python extension, whereas we would
"wait" and allow insertion of new line after detecting user's Python
command is not complete.

When the user finally types enter again on a blank line, we should just
proceed to execute whatever code, regardless of whether it is
complete/valid or not to replicate Python's original interactive REPL
behavior.

Basically creating Python command and registering that for keybinding of
'Enter'.
This would conditionally call interactive.execute which would then
eventually call our execute handler contributed from Python n
extension's REPL controller, or go ahead and insert,pass in Enter to the
text input box to allow user to type "complete" code.



This PR only intends to implement/add changes regarding execute on enter
logic, adding Intellisense support, and also adding things into
disposables so they can be properly disposed. Trying to also add setting
to allow toggling on/off to send Python command to Terminal or IW REPL
if the user is in experiment.


Handling of interrupt for windows should be on separate PR.
Test will be added later as separate PR.

---------

Co-authored-by: Courtney Webster <60238438+cwebster-99@users.noreply.github.com>
Co-authored-by: Karthik Nadig <kanadig@microsoft.com>
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 package*.json package.json and package-lock.json don't both need updating skip tests Updates to tests unnecessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants