Refactor comment parsing to use odoc-parser and cmarkit #1088

tmattio · 2023-05-08T14:01:31Z

Omd 2.X doesn't support printing markdown, so OCaml LSP is blocked to Omd 1.X. This creates conflicts with codebases that have migrated to Omd 2.X (e.g. in ocamlorg).

To avoid conflicts for users of Omd 2.X, we migrate to cmarkit, which has a smaller dependency profile and supports printing to markdown.

Not complete yet, the support for tags is incomplete and I'll add a few more tests to make sure we're not breaking anything.

cc @tatchi @ddickstein

tmattio · 2023-05-08T14:07:06Z

also cc @dbuenzli as you might be interested in this. Certainly not as solid as ocamlmark in the current state, but this essentially implements the reverse transformations you implemented in the PR on odoc-parser.

ocaml-lsp-server/test/e2e/__tests__/textDocument-signatureHelp.ts

ocaml-lsp-server/src/doc_to_md.ml

dune-project

tmattio

@3Rafal I've rebased the PR on master. I left a few comments in the tests for things that appear to be broken - we'll need to fix them before the PR can be merged.

ocaml-lsp-server/test/e2e/__tests__/textDocument-signatureHelp.ts

coveralls · 2023-05-17T12:01:18Z

Pull Request Test Coverage Report for Build 3802

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 147 (0.0%) changed or added relevant lines in 1 file are covered.
4400 unchanged lines in 7 files lost coverage.
Overall coverage increased (+0.5%) to 17.36%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ocaml-lsp-server/src/doc_to_md.ml	0	147	0.0%

Files with Coverage Reduction	New Missed Lines	%
ocaml-lsp-server/src/configuration.ml	8	45.45%
ocaml-lsp-server/src/merlin_config.ml	17	17.26%
lsp/src/server_notification.ml	20	45.45%
ocaml-lsp-server/src/progress.ml	20	5.71%
lsp/src/client_notification.ml	21	24.0%
ocaml-lsp-server/src/ocaml_lsp_server.ml	39	36.39%
lsp/src/types.ml	4275	10.55%

Totals
Change from base Build 3733:	0.5%
Covered Lines:	4073
Relevant Lines:	23462

💛 - Coveralls

Omd 2.X doesn't support printing markdown, so OCaml LSP is blocked to Omd 1.X. This creates conflicts with codebases that have migrated to Omd 2.X (e.g. in ocamlorg). To avoid conflicts for users of Omd 2.X, we migrate to cmarkit, which has a smaller dependency profile and supports printing to markdown. Co-authored-by: Rafał Gwoździński <[email protected]>

tmattio · 2023-05-17T14:01:11Z

ocaml-lsp-server/src/doc_to_md.ml

      ; location
      } ->
    let info_string =
      match metadata with
-      | None -> None
+      | None -> Some ("ocaml", loc_to_meta code_loc)


I don't think that's right, can we assume that every ocamldoc code block is in ocaml?

I assume that we default to ocaml when lang header is missing. See: https://ocaml.github.io/odoc/odoc_for_authors.html#ocaml_code_blocks

Also, I've added an explicit test case for this

tmattio · 2023-05-17T16:52:57Z

@rgrinberg this should be ready to review/merge. The support is incomplete, but there shouldn't be any regression compared to the current implementation (and we're actually fixing a couple of issues as demonstrated in the tests).

tmattio · 2023-05-17T16:57:15Z

ocaml-lsp-server/test/e2e/__tests__/textDocument-signatureHelp.ts


              #### Module List

-              * * *
-              ***@param*** \`x\` dividend
+              *Array


Module list are still broken

Ah, for some reason I thought that I should fix only commented stuff. 😅 I'll try to fix it asap.

Wow, this is more involved. It didn't work previously so we can fix it later.

I think I misunderstood the feature. The list is fixed now :)

tmattio · 2023-05-17T16:58:20Z

Spoke too soon, the lists are still broken. Should be ready to review though.

rgrinberg · 2023-05-18T08:12:12Z

@Khady can you take a look? Since you're the original author of this feature.

3Rafal · 2023-05-18T08:28:10Z

@tmattio , lists are already working. I've created an issue in cmarkit, so that it works by default in future:
dbuenzli/cmarkit#9

Khady · 2023-05-19T08:31:48Z

actually I think that @jorisgio is the original author, but I'll try to have a look at it too

Signed-off-by: Rudi Grinberg <[email protected]>

CHANGES: ## Fixes - Disable code lens by default. The support can be re-enabled by explicitly setting it in the configuration. (ocaml/ocaml-lsp#1134) - Fix initilization of `ocamlformat-rpc` in some edge cases when ocamlformat is initialized concurrently (ocaml/ocaml-lsp#1132) - Kill unnecessary `$ dune ocaml-merlin` with SIGTERM rather than SIGKILL (ocaml/ocaml-lsp#1124) - Refactor comment parsing to use `odoc-parser` and `cmarkit` instead of `octavius` and `omd` (ocaml/ocaml-lsp#1088) This allows users who migrated to omd 2.X to install ocaml-lsp-server in the same opam switch. We also slightly improved markdown generation support and fixed a couple in the generation of inline heading and module types. - Allow opening documents that were already open. This is a workaround for neovim's lsp client (ocaml/ocaml-lsp#1067) - Disable type annotation for functions (ocaml/ocaml-lsp#1054) - Respect codeActionLiteralSupport capability (ocaml/ocaml-lsp#1046) - Fix a document syncing issue when utf-16 is the position encoding (ocaml/ocaml-lsp#1004) - Disable "Type-annotate" action for code that is already annotated. ([ocaml/ocaml-lsp#1037](ocaml/ocaml-lsp#1037)), fixes [ocaml/ocaml-lsp#1036](ocaml/ocaml-lsp#1036) - Fix semantic highlighting of long identifiers when using preprocessors ([ocaml/ocaml-lsp#1049](ocaml/ocaml-lsp#1049), fixes [ocaml/ocaml-lsp#1034](ocaml/ocaml-lsp#1034)) - Fix the type of DocumentSelector in cram document registration (ocaml/ocaml-lsp#1068) - Accept the `--clientProcessId` command line argument. (ocaml/ocaml-lsp#1074) - Accept `--port` as a synonym for `--socket`. (ocaml/ocaml-lsp#1075) - Fix connecting to dune rpc on Windows. (ocaml/ocaml-lsp#1080) ## Features - Add "Remove type annotation" code action. (ocaml/ocaml-lsp#1039) - Support settings through `didChangeConfiguration` notification (ocaml/ocaml-lsp#1103) - Add "Extract local" and "Extract function" code actions. (ocaml/ocaml-lsp#870) - Depend directly on `merlin-lib` 4.9 (ocaml/ocaml-lsp#1070)

tmattio mentioned this pull request May 8, 2023

[WIP] Upgrade Omd to 2.0.0~alpha3 #1089

Closed

tmattio force-pushed the cmarkit branch from 56f456b to 60c2166 Compare May 8, 2023 18:59

tmattio mentioned this pull request May 8, 2023

test(ocamllsp): ocamldoc markdown conversion #1093

Merged

tmattio commented May 8, 2023

View reviewed changes

ocaml-lsp-server/test/e2e/__tests__/textDocument-signatureHelp.ts Outdated Show resolved Hide resolved

3Rafal mentioned this pull request May 12, 2023

Apply workaround for Block_line.list_of_string tmattio/ocaml-lsp#4

Merged

dbuenzli reviewed May 12, 2023

View reviewed changes

ocaml-lsp-server/src/doc_to_md.ml Outdated Show resolved Hide resolved

tmattio commented May 15, 2023

View reviewed changes

dune-project Outdated Show resolved Hide resolved

tmattio marked this pull request as ready for review May 17, 2023 11:47

tmattio force-pushed the cmarkit branch from 53d3703 to 5c3a858 Compare May 17, 2023 11:53

tmattio commented May 17, 2023

View reviewed changes

tmattio changed the title ~~[WIP] Refactor comment parsing to use odoc-parser and cmarkit~~ Refactor comment parsing to use odoc-parser and cmarkit May 17, 2023

tmattio force-pushed the cmarkit branch from 5c3a858 to 620034b Compare May 17, 2023 12:51

Fix comment parsing tests

d4824da

tmattio commented May 17, 2023

View reviewed changes

3Rafal added 2 commits May 17, 2023 17:13

Add test for code block language header

c9431bc

fmt

a2f91fc

tmattio commented May 17, 2023

View reviewed changes

tmattio and others added 2 commits May 17, 2023 19:00

Add a changelog entry

91a1598

Fix list indentation

8325902

3Rafal added 4 commits May 18, 2023 10:29

Update doc_to_md TODO

303d6f3

Update opam-repository in flake.lock

6b07651

Fix module lists

f059af2

Update TODO

d0fea98

rgrinberg mentioned this pull request Jun 5, 2023

Vendoring Omd 1.3.2 #1118

Closed

_

8406a9e

Signed-off-by: Rudi Grinberg <[email protected]>

rgrinberg added this to the 1.16.0 milestone Jun 8, 2023

rgrinberg merged commit fe72fab into ocaml:master Jun 8, 2023

rgrinberg mentioned this pull request Jun 18, 2023

[new release] ocaml-lsp-server, lsp and jsonrpc (1.16.0-4.14) ocaml/opam-repository#23950

Closed

tmattio mentioned this pull request Jul 18, 2023

Display text of references in doc strings #1166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor comment parsing to use odoc-parser and cmarkit #1088

Refactor comment parsing to use odoc-parser and cmarkit #1088

tmattio commented May 8, 2023

tmattio commented May 8, 2023

tmattio left a comment

coveralls commented May 17, 2023 •

edited

Loading

tmattio May 17, 2023

3Rafal May 17, 2023

tmattio commented May 17, 2023

tmattio May 17, 2023

3Rafal May 17, 2023

3Rafal May 18, 2023

3Rafal May 19, 2023

tmattio commented May 17, 2023

rgrinberg commented May 18, 2023

3Rafal commented May 18, 2023 •

edited

Loading

Khady commented May 19, 2023

Refactor comment parsing to use odoc-parser and cmarkit #1088

Refactor comment parsing to use odoc-parser and cmarkit #1088

Conversation

tmattio commented May 8, 2023

tmattio commented May 8, 2023

tmattio left a comment

Choose a reason for hiding this comment

coveralls commented May 17, 2023 • edited Loading

Pull Request Test Coverage Report for Build 3802

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

tmattio May 17, 2023

Choose a reason for hiding this comment

3Rafal May 17, 2023

Choose a reason for hiding this comment

tmattio commented May 17, 2023

tmattio May 17, 2023

Choose a reason for hiding this comment

3Rafal May 17, 2023

Choose a reason for hiding this comment

3Rafal May 18, 2023

Choose a reason for hiding this comment

3Rafal May 19, 2023

Choose a reason for hiding this comment

tmattio commented May 17, 2023

rgrinberg commented May 18, 2023

3Rafal commented May 18, 2023 • edited Loading

Khady commented May 19, 2023

coveralls commented May 17, 2023 •

edited

Loading

3Rafal commented May 18, 2023 •

edited

Loading