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

Construct custom query #1348

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Construct custom query #1348

merged 5 commits into from
Sep 27, 2024

Conversation

xvw
Copy link
Collaborator

@xvw xvw commented Jul 19, 2024

At the moment, construct is a completion hook, and in some cases accessing it via a one-off query can be useful.

@coveralls
Copy link

coveralls commented Jul 19, 2024

Pull Request Test Coverage Report for Build 4531

Details

  • 31 of 44 (70.45%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 21.931%

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_construct.ml 31 44 70.45%
Files with Coverage Reduction New Missed Lines %
ocaml-lsp-server/src/import.ml 1 34.69%
Totals Coverage Status
Change from base Build 4510: 0.08%
Covered Lines: 5588
Relevant Lines: 25480

💛 - Coveralls

@rgrinberg
Copy link
Member

When you say "can be useful", do you mean you have a concrete user in mind? Perhaps a client where you are planning to use this.

@xvw
Copy link
Collaborator Author

xvw commented Jul 19, 2024

When you say "can be useful", do you mean you have a concrete user in mind? Perhaps a client where you are planning to use this.

Using Lsp through Emacs (that does not rely to much on hover and interactivy on top of mouse) can make the usage of construct cumbersome so I'll probably rely on a custom request to use it when I'll move to emacs + lsp (instead of emacs + merlin).

@rgrinberg
Copy link
Member

I don't doubt the potential. What I'm saying is that until some client actually demonstrates real intention in using your custom request, you're just adding dead code. In the lsp standard itself, there's a requirement to demonstrate at least one client implementation for every additional protocol extension. We don't have to go that far, but how about we at least get some feedback from at least one actual client developer that is going to use this custom request before jump the gun.

@xvw
Copy link
Collaborator Author

xvw commented Jul 22, 2024

@rgrinberg
We expect different clients to use that query:

  • The Jane Street internal Emacs and Vim clients (which are not open-source). At JS, they've already enhanced the contruct query by having one code action per construct value as the current design of construct in OCaml LSP is VSCode-specifiv and can't be used by Emacs and Vim. The design of one code action per construct value isn't fully satisfying either, particularly on a long list of construct values (That's why Jane Street prefers to have a custom query).
  • We don't have concrete plans with a timeline yet, but we're certainly planning to have open-source Emacs and Vim extensions for the Emacs and Vim LSP plug-ins to support OCaml LSP specific functionalities. We're planning to use the new construct query in there as well.

@rgrinberg
Copy link
Member

Okay, so what I'm getting is that the only client developer we need to consider is from JS. So let's have somebody from there have a look at this request to do a sanity check.

@rgrinberg
Copy link
Member

cc @awilliambauer

Copy link
Contributor

@awilliambauer awilliambauer left a comment

Choose a reason for hiding this comment

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

Looks good overall, just need a few tweaks to the documentation.

ocaml-lsp-server/docs/ocamllsp/construct-spec.md Outdated Show resolved Hide resolved
ocaml-lsp-server/docs/ocamllsp/construct-spec.md Outdated Show resolved Hide resolved
ocaml-lsp-server/docs/ocamllsp/construct-spec.md Outdated Show resolved Hide resolved
ocaml-lsp-server/src/custom_requests/req_construct.ml Outdated Show resolved Hide resolved
@voodoos voodoos merged commit 671fbed into ocaml:master Sep 27, 2024
7 checks passed
@xvw xvw deleted the construct-custom-query branch September 27, 2024 09:38
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.

5 participants