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

Type Enclosing Command #14

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Type Enclosing Command #14

wants to merge 8 commits into from

Conversation

xvw
Copy link
Member

@xvw xvw commented Jan 10, 2025

The branch exposes the ocaml-eglot-type-enclosing function, which displays the type of the expression under the cursor.

  • ocaml-eglot-type-enclosing-increase-verbosity (C-c
    C-t): to increase the verbosity of the type observed
  • ocaml-eglot-type-enclosing-grow (C-↑): to grow the
    expression
  • ocaml-eglot-type-enclosing-shrink (C-↓): to shrink the
    expression
  • ocaml-eglot-type-enclosing-copy (C-w): to copy the
    type expression to the kill-ring (clipboard)

Decisions

At present, the control of the number of fines is reset to zero each time the enclosures are varied. However calling C-c C-t will increase the verbosity on the current enclosing type. It's easily possible to maintain verbosity control by ‘requests’ if you think that's better (which I don't).

More

Currently, there are no queries to ask for the type of an identifier (initially usable with the prefix argument in Merlin, ie: C-u C-c C-t will ask for an identifier, List.map and will display ('a -> “b) -> ”a list -> ’b list.

But as it's possible to search by type, I'm not sure that the feature is ‘so necessary’.

@xvw xvw force-pushed the type-enclosing-final branch from c942c57 to c624581 Compare January 10, 2025 17:54
@xvw xvw force-pushed the type-enclosing-final branch from c624581 to 4a3ff5c Compare January 10, 2025 17:55
@xvw xvw requested a review from voodoos January 10, 2025 17:55
@gr-im
Copy link

gr-im commented Jan 12, 2025

At present, the control of the number of fines is reset to zero each time the enclosures are varied

WDYT about keeping verbosity from enclosings to enclosings? And restoring it to 0 only when re-triggering the command? (and offering a transient binding for C-<left> and C-<right> to increase/decrease verbosity for current enclosing?)

@voodoos
Copy link
Member

voodoos commented Jan 13, 2025

I had similar thoughts when integrating the feature in vscode and I don't have a strong preference:

  • The current behavior is the same as the existing client (I think), so memory muscle will be rewarded.
  • The proposed 2D navigation scheme with the arrows and verbosity memory does seems nicer.

I mean, it's probably the best time to introduce this kind of UX changes, as long as we keep the "multiple presses on C-t augments verbosity"...

@xvw
Copy link
Member Author

xvw commented Jan 13, 2025

@gr-im, @voodoos
Behaviour appended on 48c95e0

Copy link
Member

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

Just some doc comments right now, I will try to do a more in depth-review tomorrow.

We should really pay attention to the docstring of interactive functions :-)

Comment on lines +43 to +44
`ocaml-eglot` displays the type of expression below the cursor and
increases or decreases the enclosings of the expression or verbosity:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`ocaml-eglot` displays the type of expression below the cursor and
increases or decreases the enclosings of the expression or verbosity:
In `ocaml-eglot` one can display the type of the expression below the cursor and
navigate the enclosing nodes while increasing or decreasing verbosity:

Comment on lines +46 to +48
- `ocaml-eglot-type-enclosing` (<kbd>C-c</kbd> <kbd>C-t</kbd>)

And when a type is displayed, additional commands are possible:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `ocaml-eglot-type-enclosing` (<kbd>C-c</kbd> <kbd>C-t</kbd>)
And when a type is displayed, additional commands are possible:
- `ocaml-eglot-type-enclosing` (<kbd>C-c</kbd> <kbd>C-t</kbd>)
Display the type of the selection and start a "type enclosing" session.
During a "type enclosing" session the following commands are available:

;; Type Enclosings

(defun ocaml-eglot-type-enclosing ()
"A needed documentation."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"A needed documentation."
"A very much needed documentation."

Maybe you can take inspiration from: https://github.com/voodoos/merlin/blob/project-wide-occurrences/emacs/merlin.el

@@ -93,6 +93,10 @@ Otherwise, `merlin-construct' only includes constructors."
"Face describing the doc of values (used for search for example)."
:group 'ocaml-eglot)

(defface ocaml-eglot-highlight-region-face
'((t (:inherit highlight)))
"Face for highlighting a region.")
Copy link
Member

Choose a reason for hiding this comment

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

Is a face a font or police/style ? Then I think Face used when highlighting a region. would be clearer.

(setq ocaml-eglot-type-enclosing-offset 0))

(defun ocaml-eglot-type-enclosing--call ()
"Prepare the type-enclosings computation request."
Copy link
Member

Choose a reason for hiding this comment

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

I think the feature is named type-enclosing without an s.
But what it does is returning the type of enclosings with an s.

Suggested change
"Prepare the type-enclosings computation request."
"Prepare the type-enclosing computation request."

Copy link
Member

Choose a reason for hiding this comment

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

Also, it's not only a preparation, but it displays a type to the user. This function and the other user-facing one should describe as clearly as possible what they are doing.

(ocaml-eglot-type-enclosing--with-fixed-offset)))

(defun ocaml-eglot-type-enclosing-shrink ()
"Shrinking of the type enclosing."
Copy link
Member

Choose a reason for hiding this comment

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

Hum, I am not sure of the best wording here... same for the others.

Suggested change
"Shrinking of the type enclosing."
"Display the type enclosing of a smaller enclosing if possible."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants