More functionnalities for repl #951

OscarLahaie · 2022-05-11T09:36:43Z

This pr is intended to be a part of the work of @ulugbekna in #677.
It would be interesting to have an evaluation system similar to Tuareg in Emacs. For this it is not only necessary to evaluate the user's selection but also when there is no selection the expression corresponding to the cursor position.

The objectives :

Evaluate a file
Preserve focus on editor after evaluations
Evaluate expressions
Option to jump to the next expression

OscarLahaie · 2022-05-11T09:38:30Z

This is the cleaner version of pull request #943.

mnxn · 2022-05-26T00:40:58Z

src/repl.ml

+  if is_repl_ready code then
+    (* newline is for nicer look in REPL *)
+    "\n" ^ trimmed_code
+  else Printf.sprintf "\n%s\n;;" trimmed_code


This formatting sometimes looks worse like in this example with a single expression:

# x ;; - : int = 1

Maybe it's worth checking if the code is a single line first? If it is, then there's no need to add any extra formatting.

I would prefer the following in simple cases:

# x ;; - : int = 1

Done!
I have done some tests and it seems to work well with the new version of the function.
I am not sure about the List.mem efficiency, what do you think about this solution?

The new behavior is great.

String.mem is fine but I'm not sure what the difference is from String.contains that we use in other parts of the extension.

mnxn

A few changes need to be made for Windows and the official OCaml REPL.

I haven't had a chance to try macOS or utop yet.

src/repl.ml

OscarLahaie · 2022-05-26T20:58:51Z

I made several tests to see if the jump is working but I am sure some cases unknown to me.
So if you find any bugs tell me so I can fix them.

rgrinberg · 2022-06-11T13:59:34Z

@mnxn is this good to merge?

ulugbekna · 2022-06-11T14:38:28Z

I will have a look at this today

mnxn · 2022-06-11T19:52:34Z

is this good to merge?

It looked good to me when I tried it.

Ready to merge unless @ulugbekna finds something.

Add the functionnality to auto select the expression instead of the line if the selection is empty

Now handles single lines and fixe the `is_repl_ready` check

In previous versions, the cursor position was sent without worrying about whether it was an expression. Now we send the position so that the cursor is in the previous expression.

ulugbekna

This needs a changelog entry.

Didn't look too closely at the 180-line function, but functionality added by the PR seems to work well

ulugbekna · 2022-06-11T23:33:09Z

src/repl.ml

@@ -165,7 +183,188 @@ module Command = struct
    in
    Extension_commands.register ~id:Extension_consts.Commands.open_repl handler

-  let _evaluate_selection =
+  let _evaluate_expression =


I'm a little skeptical about having a 180-line function

* some cleanups

ulugbekna · 2022-06-12T10:21:33Z

I think this can me squashed and merged, but jumping to next expression implementation needs to be made on the ocamllsp side as a new custom request so that we avoid hacks that we have now.

Also, one should be careful using registerTextEditorCommand because it's supposed to be run synchronously, meaning starting off a promise as we do it now is unsafe. See microsoft/vscode#16814

OscarLahaie · 2022-06-12T10:39:31Z

I think this can me squashed and merged, but jumping to next expression implementation needs to be made on the ocamllsp side as a new custom request so that we avoid hacks that we have now.

Also, one should be careful using registerTextEditorCommand because it's supposed to be run synchronously, meaning starting off a promise as we do it now is unsafe. See microsoft/vscode#16814

I don't have much time this week. Nevertheless I will look at and improve the code in the coming weeks if necessary. Because there are some behaviours to be fixed at the ocaml-lsp level.

rgrinberg · 2022-06-14T14:38:33Z

src/repl.ml

-    Vscode.TextLine.text line
+let get_selected_code text_editor =
+  let selection = TextEditor.selection text_editor in
+  let document = TextEditor.document text_editor in


Why do you obtain document right away but only use it in one branch of the conditional?

rgrinberg · 2022-06-14T14:40:05Z

src/repl.ml

+  let is_repl_ready s = String.is_suffix s ~suffix:";;" in
+  let trimmed_code = String.strip code in
+  if is_repl_ready trimmed_code then
+    if String.mem trimmed_code '\n' then


As far as I can tell String.mem trimmed_code '\n' is always evaluated. Therefore it should be assigned a binding and moved before the conditionals.

rgrinberg · 2022-06-14T14:42:58Z

README.md

+| `ocaml.evaluate-file`        | Evaluate Selection                          | `Shift+Alt+Enter`  |
+
+Note: `ocaml.evaluate-expression` was previously named
+`ocaml.evaluate-selection`. You can also disabe the new cursor jumps after this


Why don't we preserve ocaml.evaluate-selection? I don't see anything wrong with having both commands.

the new command evaluates the selection if there's one; otherwise, it evaluates a (module-level) let or module binding depending which comes first

I understood. I just don't see why we should remove a command with a clear name and semantics and replace it with a command that has neither of those things. Seems like we're just irritating users who are used to the old command this way.

Good point, but then I would show a deprecation message saying to use the new command because

The new command has almost the same semantics - select and evaluate (no selection resulted in evaluating the whole line but I'm not sure that's very useful and the new command does the same thing if it's a single-line let binding)

There's then contention for the shortcut: keep the old and miss out on a better command or replace and still irritate

rgrinberg · 2022-06-14T14:57:07Z

Before merging this, I think we need to get on the same page of what good REPL functionality looks like. I have no interest in copying tuareg and its limitations. We can do a lot better if we think about things a bit more.

ulugbekna · 2022-06-14T15:04:11Z

Before merging this, I think we need to get on the same page of what good REPL functionality looks like. I have no interest in copying tuareg and its limitations. We can do a lot better if we think about things a bit more.

Do you have precise limitations you'd like gone before merging?

Things that are not addressed in this PR but can be done in other, IMO:

configuration for how module is evaluated (#use or #mod_use) as a dialog
configurable REPL instance -- the best decision is to have two commands for picking a terminal to send commands to:
- one command to pick an integrated terminal that already exists from a list (this allows users to jump into existing repl session and send commands to it, for example)
- one command to launch a terminal from settings-defined repl binary and args OR default utop - this already exists but it uses a timeout before sending commands to a new REPL -- this needs to be fixed

rgrinberg · 2022-06-14T15:51:35Z

First of all, as a general comment, there's just too many knobs, settings, commands that all make the experience way too complicated. I've already went through this stuff with tuareg mode. In the end, their toplevel integration is hardly usable.

99% of the time when I use the toplevel, I do the following:

Open a project
Navigate to a module I want to experiment with
Figure out how to load into the toplevel.

We should focus on this core feature first before we worry about directives, selecting repl instances. Finally, we should absolutely avoid any code that tries to parse OCaml. I see this PR is adding quite a bit of that and it's really wasted effort.

Here's a simple idea for how to make the toplevel more useful.

We add a command $ dune ocaml top-module path/to/module.ml --source=/tmp/selection/file. This command will either load the toplevel binary or print the directives required to load module.ml as if its source was --source.
We add a request to lsp that given a position in the document, will return a completed & pretty printed parse tree. Luckily for us, merlin already knows how to do this.
On the vscode side, we add a command to launch a toplevel where the cursor is. It will obtain the source using lsp, then run the dune command mentioned above. There's no need to select toplevel instances. We associate every module with a toplevel instance. If there's already a toplevel for a module, then we kill it and launch it again.

Let's forget about all the other stuff for now.

rgrinberg · 2022-06-14T16:01:05Z

src/repl.ml

+                    ~position:correct_position
+                in
+                match range with
+                | None -> ()


When can this request return None?

rgrinberg · 2022-06-14T16:06:11Z

A few more thoughts:

When there's no up to date lsp server running, the user should only be able to send selections.
Requests such as the wrapping ast node should just select the range they're returning. The user can send it manually.
Moving to the next thing to evaluate should be a separate command. If someone wants to combine evaluating and moving, they can create their own keyboard shortcut for it.

rgrinberg requested review from ulugbekna and mnxn May 17, 2022 20:18

mnxn reviewed May 26, 2022

View reviewed changes

mnxn requested changes May 26, 2022

View reviewed changes

src/repl.ml Outdated Show resolved Hide resolved

src/repl.ml Outdated Show resolved Hide resolved

OscarLahaie marked this pull request as draft May 26, 2022 12:39

OscarLahaie marked this pull request as ready for review May 26, 2022 16:14

OscarLahaie requested a review from mnxn May 26, 2022 16:14

mnxn approved these changes May 26, 2022

View reviewed changes

OscarLahaie added 16 commits June 12, 2022 00:50

Evaluate file

262d495

Preserve focus

331577c

Remove location message

4d769f8

get_code -> get_selected_code

9f3e328

prepare_code -> preformat_code

d0e0bbd

Wrapping Ast Node support

880864c

evaluate-selection -> evaluate-expression

bcc8dc3

Add the functionnality to auto select the expression instead of the line if the selection is empty

better preformat

76736da

Now handles single lines and fixe the `is_repl_ready` check

Update path request and usage

5612c5b

Name change : _evaluate_selection -> _evaluate_expression

eb22999

Fix the position of cursor send to lsp-server

fc62f64

In previous versions, the cursor position was sent without worrying about whether it was an expression. Now we send the position so that the cursor is in the previous expression.

Infinity loop fix

9c2802b

Add comments case

76dffe6

Fix top call problem

c7b99ea

Cursor jump after expressions evaluations

9581a42

Readme update

8d1ca2d

OscarLahaie and others added 3 commits June 12, 2022 00:50

Now handles comments between expressions

0e0c694

Better end file behavior

6a363ca

remove article

17952b7

ulugbekna requested changes Jun 11, 2022

View reviewed changes

ulugbekna force-pushed the better-repl branch from e9cc637 to 17952b7 Compare June 11, 2022 23:36

Ulugbek added 4 commits June 12, 2022 11:15

rename commands specifying that evaluation happens in REPL

e762957

make fmt

8f4c068

* reveal new selection when cursor jumps;

005ef00

* some cleanups

some more cleanup

1b9c5b6

rgrinberg requested changes Jun 14, 2022

View reviewed changes

rgrinberg reviewed Jun 14, 2022

View reviewed changes

src/repl.ml

~position:correct_position

in

match range with

| None -> ()

Copy link

Contributor

rgrinberg Jun 14, 2022

Choose a reason for hiding this comment

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

When can this request return None?

faldor20 mentioned this pull request Jul 6, 2023

Preserve focus when sending to repl #1161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More functionnalities for repl #951

More functionnalities for repl #951

OscarLahaie commented May 11, 2022 •

edited

OscarLahaie commented May 11, 2022

mnxn May 26, 2022

OscarLahaie May 26, 2022

mnxn May 26, 2022

mnxn left a comment •

edited

OscarLahaie commented May 26, 2022

rgrinberg commented Jun 11, 2022

ulugbekna commented Jun 11, 2022

mnxn commented Jun 11, 2022 •

edited

ulugbekna left a comment

ulugbekna Jun 11, 2022

ulugbekna commented Jun 12, 2022 •

edited

OscarLahaie commented Jun 12, 2022

rgrinberg Jun 14, 2022

rgrinberg Jun 14, 2022

rgrinberg Jun 14, 2022

ulugbekna Jun 14, 2022

rgrinberg Jun 14, 2022

ulugbekna Jun 14, 2022

rgrinberg commented Jun 14, 2022

ulugbekna commented Jun 14, 2022

rgrinberg commented Jun 14, 2022

rgrinberg Jun 14, 2022

rgrinberg commented Jun 14, 2022

More functionnalities for repl #951

Are you sure you want to change the base?

More functionnalities for repl #951

Conversation

OscarLahaie commented May 11, 2022 • edited

OscarLahaie commented May 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnxn left a comment • edited

Choose a reason for hiding this comment

OscarLahaie commented May 26, 2022

rgrinberg commented Jun 11, 2022

ulugbekna commented Jun 11, 2022

mnxn commented Jun 11, 2022 • edited

ulugbekna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulugbekna commented Jun 12, 2022 • edited

OscarLahaie commented Jun 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Jun 14, 2022

ulugbekna commented Jun 14, 2022

rgrinberg commented Jun 14, 2022

Choose a reason for hiding this comment

rgrinberg commented Jun 14, 2022

OscarLahaie commented May 11, 2022 •

edited

mnxn left a comment •

edited

mnxn commented Jun 11, 2022 •

edited

ulugbekna commented Jun 12, 2022 •

edited