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.

Sign up for GitHub

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 to bottom

Documentation custom query #1336

Merged
merged 23 commits into from
Jul 30, 2024
Merged

Documentation custom query #1336

merged 23 commits into from
Jul 30, 2024

Conversation

PizieDust
Copy link
Contributor

closes #1319

Add documentation custom query.

@coveralls
Copy link

coveralls commented Jul 9, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 4469

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 28 of 41 (68.29%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 21.895%
Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_get_documentation.ml 28 41 68.29%
Totals Coverage Status
Change from base Build 4448: 0.2%
Covered Lines: 5571
Relevant Lines: 25444
💛 - Coveralls

xvw
xvw requested changes Jul 9, 2024
View reviewed changes
Copy link
Collaborator

@xvw xvw left a comment

Choose a reason for hiding this comment

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

Very nice work!
I think that we can improve a little bit the code organization exposing only a Request_params module (for the input arguments) and exposing a type (t) that describes the output of the query.
I think that we can also only "produces parameters" into the body of the Fiber.thrunk and using a dedicated function to fetching the documentation.

ocaml-lsp-server/src/custom_requests/req_documentation.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/custom_requests/req_documentation.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/custom_requests/req_documentation.mli Outdated Show resolved Hide resolved
ocaml-lsp-server/src/custom_requests/req_documentation.mli Outdated Show resolved Hide resolved
ocaml-lsp-server/src/custom_requests/req_documentation.mli Outdated Show resolved Hide resolved
ocaml-lsp-server/test/e2e-new/documentation.ml Show resolved Hide resolved
@PizieDust PizieDust requested a review from xvw July 16, 2024 07:36
@PizieDust PizieDust requested a review from xvw July 18, 2024 06:33
@PizieDust PizieDust requested a review from rgrinberg July 18, 2024 06:33
@PizieDust PizieDust requested a review from xvw July 18, 2024 13:13
@PizieDust PizieDust requested a review from xvw July 18, 2024 13:29
xvw
xvw approved these changes Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@xvw xvw left a comment

Choose a reason for hiding this comment

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

Approved! Thanks a lot @PizieDust!

@PizieDust
Copy link
Contributor Author

Approved! Thanks a lot @PizieDust!

Thanks for the reviews

@xvw
Copy link
Collaborator

xvw commented Jul 19, 2024

@rgrinberg Do you want to make one last pass?

@rgrinberg
Copy link
Member

The implementation looks fine. @awilliambauer do you want to take a look at the docs? Is this something that you will be able to use?

awilliambauer
awilliambauer reviewed Jul 22, 2024
View reviewed changes
ocaml-lsp-server/docs/ocamllsp/getDocumentation-spec.md Outdated
```
- method : `ocamllsp/getDocumentation`
- params :
- `position`: The position of the cursor.
Copy link
Contributor

Choose a reason for hiding this comment

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

position does not appear in the object type above - is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since the interface itself extends TextDocumentPositionParams which contains both the Document Uri and the Position

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably provide a link to the TextDocumentPositionParams and remove the description of position (since textDocument, the TextDocumentIdentifier is not mentionned in the documentation).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure that it is right. I think that you should explain that the param should be a TextDocumentPositionParams (with a link to the specification) with additonal ones, enumerated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xvw I just pushed a commit with some documentation changes. Can you check if it's more structured?

@awilliambauer
Copy link
Contributor

This looks reasonable to me, and useful for providing documentation-focus functionality.

voodoos
voodoos reviewed Jul 29, 2024
View reviewed changes
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.

This PR looks good, thanks @PizieDust

I think we do miss one thing however: when markdown is requested we should use the Doc_to_md module to translate ocamldoc syntax to markown.

ocaml-lsp-server/src/custom_requests/req_get_documentation.ml Outdated Show resolved Hide resolved
@PizieDust PizieDust requested a review from voodoos July 29, 2024 09:51
voodoos
voodoos approved these changes Jul 29, 2024
View reviewed changes
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.

Thanks for the change ! I think this is ready then ?

@voodoos voodoos merged commit 8891959 into ocaml:master Jul 30, 2024
8 checks passed
voodoos added a commit that referenced this pull request Jul 30, 2024
@voodoos
Fix changelog after merging #1336
c91c19c
voodoos added a commit to voodoos/opam-repository that referenced this pull request Jul 30, 2024
@voodoos
[new release] lsp (3 packages) (1.19.0)
ed38a8b 
CHANGES:

## Features

- Add custom [`ocamllsp/getDocumentation`](/ocaml-lsp-server/docs/ocamllsp/getDocumentation-spec.md) request (ocaml/ocaml-lsp#1336)
- Add support for OCaml 5.2 (ocaml/ocaml-lsp#1233)

## Fixes

- Kill unnecessary ocamlformat processes with sigterm rather than sigint or
  sigkill (ocaml/ocaml-lsp#1343)
@voodoos voodoos mentioned this pull request Jul 30, 2024
Merged
voodoos added a commit to voodoos/opam-repository that referenced this pull request Jul 30, 2024
@voodoos
[new release] lsp (3 packages) (1.19.0)
99f1a14 
CHANGES:

## Features

- Add custom [`ocamllsp/getDocumentation`](/ocaml-lsp-server/docs/ocamllsp/getDocumentation-spec.md) request (ocaml/ocaml-lsp#1336)
- Add support for OCaml 5.2 (ocaml/ocaml-lsp#1233)

## Fixes

- Kill unnecessary ocamlformat processes with sigterm rather than sigint or
  sigkill (ocaml/ocaml-lsp#1343)
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
@voodoos @avsm
[new release] lsp (3 packages) (1.19.0)
a07ed54 
CHANGES:

## Features

- Add custom [`ocamllsp/getDocumentation`](/ocaml-lsp-server/docs/ocamllsp/getDocumentation-spec.md) request (ocaml/ocaml-lsp#1336)
- Add support for OCaml 5.2 (ocaml/ocaml-lsp#1233)

## Fixes

- Kill unnecessary ocamlformat processes with sigterm rather than sigint or
  sigkill (ocaml/ocaml-lsp#1343)
@PizieDust PizieDust mentioned this pull request Oct 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awilliambauer awilliambauer awilliambauer left review comments

@xvw xvw xvw approved these changes

@voodoos voodoos voodoos approved these changes

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documentation query
6 participants
@PizieDust @coveralls @xvw @rgrinberg @awilliambauer @voodoos