Jump to holes ocamllsp typed holes

[ulugbekna]

Uses a custom request in ocamllsp to jump around holes. This should allow to eliminate too tight coupling with diagnostics, which caused problems when we want to jump to the first hole in a destructed code.

[ulugbekna]

I have better things to do sorry

…

On Fri, Jun 25, 2021, 20:52 Max Lantas ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/extension_commands.ml <#643 (comment)> : > @@ -131,6 +133,124 @@ let _open_current_dune_file = in command Extension_consts.Commands.open_current_dune_file handler +let _jump_to_prev_hole, _jump_to_next_hole = + let pick_next_hole current_pos ~sorted_non_empty_holes_lst:holes = You can make this ~sorted_non_empty_holes_list. Omitting a single letter doesn't change much. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#643 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4YR6643XWA7WREPQLFNDTTUSQ5NANCNFSM47H5IJZA> .

[tmattio]

I have better things to do sorry

I can think of better ways to convey your point @ulugbekna 🙂

Let's assume best intentions here: Max is a valuable maintainer of vscode-ocaml-platform and his reviews are equally valuable.
He took the time to review the PR, so let's respect his time as much as I'm sure he respects the time of other contributors.

In particular, his suggestion (I'm sure he didn't feel strongly about it and meant it more as a suggestion than a blocker) on the function naming follows previous guidelines of the codebase, which ensure that the code stays readable for newcomers.
An example of a codebase that does not follow similar guidelines is Opam which I personally find very hard to get into.
So, surely, there are some solid arguments that can be made on the importance of function and variable naming.

I don't have a specific opinion on the above, but if there is a disagreement on a review, let's keep it friendly and professional. We all have the same goal here.

[ulugbekna]

I apologize to @mnxn if that sounded offensive. I assumed that his tone conveyed a suggesting character, which I turned down. Putting/omitting that character didn't change anything imho. I fully addressed the first round of comments, if necessary putting Max's suggestions/preferences above mine, e.g., using a named arg vs a type. I do, however, prefer to avoid bikeshed changes. I restrain myself from commenting on naming unless it would substantially make code easier to read. Nevertheless, I'm sorry if my comment above came off unprofessional.

I very much appreciate Max's work and how supportive he is of this repo. For example, I recall offering to implement a feature (reason config option that is?) Max brought up in one of the meetings if he himself needed that feature because I do care. And I will surely be happy to review any of Max's PRs or contribute to them.

---

The PR should be ready but still dependent on ocaml/ocaml-lsp#467. We should probably wait. I will comment in this PR when it's merged.

[mnxn]

Thank you for your apology, but I had no hard feelings. I understand that even though we are communicating in English, there can still be language differences. I know that I've had difficulties communicating my points before. I was more confused by the comment than anything, so I appreciate your clarification.

[rgrinberg]

I only briefly glance through the code, but given that Max reviewed this, it should be good to go. @mnxn did you try using this feature yet? We need to make sure the UI is solid too.

[mnxn]

did you try using this feature yet?

I have and it works well. I have not noticed any problems while using it.

[ulugbekna]

Jumping around holes using alt+l (for next hole) and alt+shift+l (for previous hole). Also showing the hole diagnostic at the beginning of the gif.

Screen.Recording.2021-06-30.at.11.19.34.mov

[rgrinberg]

Do you want to clean up the history yourself or shall I just squash everything?

[ulugbekna]

Right, I will do that manually. Meanwhile, I don't know why tests are failing because of esy.

…

On Wed, 30 Jun 2021 at 21:11, Rudi Grinberg ***@***.***> wrote: Do you want to clean up the history yourself or shall I just squash everything? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#643 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4YR65BCWMIU5W3QDUDBT3TVM62JANCNFSM47H5IJZA> .