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

Locate using lsp queries #13

Merged
merged 12 commits into from
Jan 6, 2025
Merged

Locate using lsp queries #13

merged 12 commits into from
Jan 6, 2025

Conversation

xvw
Copy link
Member

@xvw xvw commented Jan 2, 2025

The implementation of find/locate is by default backed by xref, which is very limited in terms of customization (for example, concerning definition or declaration control). This PR reimplements the Locate command by directly invoking LSP requests.

@xvw xvw requested a review from voodoos January 2, 2025 22:52
@xvw xvw self-assigned this Jan 2, 2025
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.

Looks good !

I have two remarks:

  • Go to type-declaration is an actual user action, not a fallback for when we fail to find a definition
  • Your mix and matching find and locate in the various utility functions. We should settle on one verb! I guess locate is Merlin's terminology while find is LSP's. I will let you chose the one you prefer :-)

ocaml-eglot-util.el Outdated Show resolved Hide resolved
ocaml-eglot.el Outdated Show resolved Hide resolved
ocaml-eglot.el Outdated Show resolved Hide resolved
@xvw xvw changed the title Locate using lsp queries Draftt: Locate using lsp queries Jan 6, 2025
@xvw xvw changed the title Draftt: Locate using lsp queries Draft: Locate using lsp queries Jan 6, 2025
@xvw xvw changed the title Draft: Locate using lsp queries Locate using lsp queries Jan 6, 2025
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.

LGTM. I proposed some documentation rewording.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
ocaml-eglot-util.el Outdated Show resolved Hide resolved
ocaml-eglot.el Outdated Show resolved Hide resolved
ocaml-eglot.el Outdated Show resolved Hide resolved
ocaml-eglot.el Outdated Show resolved Hide resolved
ocaml-eglot.el Outdated Show resolved Hide resolved
ocaml-eglot.el Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ocaml-eglot.el Show resolved Hide resolved
@xvw xvw force-pushed the locate-using-lsp-queries branch from 75a40c6 to 441b617 Compare January 6, 2025 15:59
@xvw xvw merged commit 1578210 into main Jan 6, 2025
4 checks passed
@xvw xvw deleted the locate-using-lsp-queries branch January 6, 2025 16:06
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.

2 participants