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

Jump Custom Request #1374

Merged
merged 16 commits into from
Nov 28, 2024
Merged

Jump Custom Request #1374

merged 16 commits into from
Nov 28, 2024

Conversation

PizieDust
Copy link
Contributor

@PizieDust PizieDust commented Sep 18, 2024

related to #1360
This PR contains an implementation for a custom requests based on Merlin's jump command.

Using code actions introduces some noise as referenced in this comment: #1364 (comment)

These code actions add a lot of clutter to the code actions menu. Would there be a way to suppress them from being displayed in the menu, but still make them available via keyboard shortcut?
There are various ways editors might want to incorporate these jump commands into a large commend (e.g., a macro that jumps to a let and adds a specific annotation). We'd also want the user to be able to navigate the history of these jumps (i.e., jump backwards and forwards). Having these as code actions instead of a dedicated custom query will probably make supporting both of these things a lot harder.

An alternative solution will be to use a custom request, which allows for more better finetuning.

Merlin Jump Request

Description

This custom request allows Merlin-type code navigation in a source buffer.

Server capability

  • propert name: handleMerlinJump
  • property type: boolean

Request

export interface JumpParams extends TextDocumentPositionParams
{
    target: string;
}
  • method: ocamllsp/merlinJump
  • params:
    • TextDocumentIdentifier: Specifies the document for which the request is sent. It includes a uri property that points to the document.
    • Position: Specifies the position in the document for which the documentation is requested. It includes line and character properties.
      More details can be found in the TextDocumentPositionParams - LSP Specification.
    • Target: A string representing the identifier within the document to search for and jump to.
      The target can be any of fun, let, module, module-type, match, match-next-case, match-prev-case.

Response

result: Jump | String
export interface Jump extends TextDocumentPositionParams {
}
  • result:
    • Type: Jump or string
    • Description: If the jump is successful, a position and document path is returned. If no relevant jump location is found, the result will be a string "no matching target" or an error message.
    • Jump:
      • Type: TextDocumentPositionParams
        • Position: The position to jump to
        • TextDocumentIdentifier: the document path which contains this position (ideally the same document as the request)

cc @voodoos @xvw @rgrinberg @awilliambauer

@coveralls
Copy link

coveralls commented Sep 18, 2024

Pull Request Test Coverage Report for Build 4624

Details

  • 30 of 32 (93.75%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 22.132%

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_merlin_jump.ml 30 32 93.75%
Totals Coverage Status
Change from base Build 4617: 0.09%
Covered Lines: 5632
Relevant Lines: 25447

💛 - Coveralls

@PizieDust PizieDust requested a review from voodoos September 18, 2024 13:21
@voodoos
Copy link
Collaborator

voodoos commented Sep 18, 2024

We should think about what we want to do with the jump code action. The two obvious possibilities are:

  1. Revert the changes
  2. Add an option to disable the jump code actions

I am personally in favor of 2, since the nice trick allows direct support in any editor with the required capabilities.
It could be enabled by default in the community version of the server/plugin, until we ourselves implement an alternative way using the custom request.

@PizieDust
Copy link
Contributor Author

We should think about what we want to do with the jump code action. The two obvious possibilities are:

  1. Revert the changes
  2. Add an option to disable the jump code actions

I am personally in favor of 2, since the nice trick allows direct support in any editor with the required capabilities. It could be enabled by default in the community version of the server/plugin, until we ourselves implement an alternative way using the custom request.

I am also in favor of 2.
I am currently working on the option to disable the jump code actions.

Copy link
Collaborator

@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.

These changes looks ok to me. It's one more custom request as discussed with @awilliambauer , but hopefully we will be able to work on client implementations soon.

CHANGES.md Outdated Show resolved Hide resolved
@PizieDust PizieDust requested a review from voodoos October 1, 2024 14:14
@voodoos
Copy link
Collaborator

voodoos commented Nov 27, 2024

This should be ready to merge now, and we have the vscode implementation ready: ocamllabs/vscode-ocaml-platform#1654

@voodoos voodoos merged commit b363a35 into ocaml:master Nov 28, 2024
5 of 7 checks passed
@PizieDust PizieDust deleted the jump_cq branch November 28, 2024 16:23
voodoos added a commit to voodoos/opam-repository that referenced this pull request Nov 29, 2024
CHANGES:

## Features

- Add custom
  [`ocamllsp/typeSearch`](/ocaml-lsp-server/docs/ocamllsp/typeSearch-spec.md) request (ocaml/ocaml-lsp#1369)

- Make MerlinJump code action configurable (ocaml/ocaml-lsp#1376)

- Add custom [`ocamllsp/jump`](/ocaml-lsp-server/docs/ocamllsp/merlinJump-spec.md) request (ocaml/ocaml-lsp#1374)

## Fixes

- Fix fd leak in running external processes for preprocessing (ocaml/ocaml-lsp#1349)

- Fix prefix parsing for completion of object methods (ocaml/ocaml-lsp#1363, fixes ocaml/ocaml-lsp#1358)

- Remove some duplicates in the `selectionRange` answers (ocaml/ocaml-lsp#1368)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Nov 29, 2024
CHANGES:

## Features

- Add custom
  [`ocamllsp/typeSearch`](/ocaml-lsp-server/docs/ocamllsp/typeSearch-spec.md) request (ocaml/ocaml-lsp#1369)

- Make MerlinJump code action configurable (ocaml/ocaml-lsp#1376)

- Add custom [`ocamllsp/jump`](/ocaml-lsp-server/docs/ocamllsp/merlinJump-spec.md) request (ocaml/ocaml-lsp#1374)

## Fixes

- Fix fd leak in running external processes for preprocessing (ocaml/ocaml-lsp#1349)

- Fix prefix parsing for completion of object methods (ocaml/ocaml-lsp#1363, fixes ocaml/ocaml-lsp#1358)

- Remove some duplicates in the `selectionRange` answers (ocaml/ocaml-lsp#1368)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Nov 29, 2024
CHANGES:

## Features

- Add custom
  [`ocamllsp/typeSearch`](/ocaml-lsp-server/docs/ocamllsp/typeSearch-spec.md) request (ocaml/ocaml-lsp#1369)

- Make MerlinJump code action configurable (ocaml/ocaml-lsp#1376)

- Add custom [`ocamllsp/jump`](/ocaml-lsp-server/docs/ocamllsp/merlinJump-spec.md) request (ocaml/ocaml-lsp#1374)

## Fixes

- Fix fd leak in running external processes for preprocessing (ocaml/ocaml-lsp#1349)

- Fix prefix parsing for completion of object methods (ocaml/ocaml-lsp#1363, fixes ocaml/ocaml-lsp#1358)

- Remove some duplicates in the `selectionRange` answers (ocaml/ocaml-lsp#1368)
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.

4 participants