Skip to content

Conversation

@Leonidas-from-XIV
Copy link
Collaborator

This is a revival of #339 with a bit of rebasing and a tiny bit more polish.

@Leonidas-from-XIV
Copy link
Collaborator Author

odoc-parser.1.0.0 is out, but unfortunately it conflicts with ocamlformat.0.19.0, so it forces a formatter downgrade which then has issues formatting our code.

@Julow
Copy link
Collaborator

Julow commented Dec 13, 2021

OCamlformat 0.20.1 has been released today and has compatibility with odoc-parser 1.0.0.

@Leonidas-from-XIV Leonidas-from-XIV marked this pull request as ready for review December 13, 2021 13:35
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

The tests need to be promoted:

diff --git a/test/bin/mdx-test/expect/simple-mli/test-case.mli b/test/bin/mdx-test/expect/simple-mli/test-case.mli
index 254d76d..78c970e 100644
--- a/test/bin/mdx-test/expect/simple-mli/test-case.mli
+++ b/test/bin/mdx-test/expect/simple-mli/test-case.mli
@@ -47,5 +47,5 @@ val foo : string
 (** {@ocaml[1 + 1 = 3]} *)
 val bar : string
 
-(** {@ocaml skip [1 + 1 = 3]} *)
+(** {@ocaml skip[1 + 1 = 3]} *)
 val baz : string
diff --git a/test/lib/test_mli_parser.ml b/test/lib/test_mli_parser.ml
index 89a6ab5..c226f93 100644
--- a/test/lib/test_mli_parser.ml
+++ b/test/lib/test_mli_parser.ml
@@ -101,7 +101,7 @@ let test_parse_mli =
       ~expected:
         (Ok
            {x|[Text "(** This doc comment with a label should get parsed\n\n    ";
- Text "{@"; Text "ocaml"; Text " "; Text "skip "; Text "[";
+ Text "{@"; Text "ocaml"; Text " "; Text "skip"; Text "[";
  Block {loc: File "_none_", lines 3-6; section: None; labels: [skip];
         header: Some ocaml; contents: ["# 1 + 1"; "- : int = 2"];
         value: Toplevel};

Julow and others added 11 commits January 6, 2022 12:07
The language tag is used as the header and the other field is for
labels.

If the metadata is absent, the code block is run as OCaml. This is
backward compatible.

Until now, Mdx would strip it from the original file by ignoring it at
parsing and not outputting it back.

This is a rebased version of #339 upon which improvements applied.

Resolves #339
@Leonidas-from-XIV
Copy link
Collaborator Author

I'm not happy that all the blocks need to know whether they are Syntax.Mli or not to print the proper padding because it it incredibly fussy and the cyclomatic complexity is through the roof but it is… not worse than before and at least Cram.t in .mli files should be displayed properly.

I suggest revisiting our Markdown parsering some time down the road to make it more similar to what we get from odoc-parser and try to avoid all this switching logic but for now I think this is ok to be reviewed and actually also fixes the Cram.t printing bug that @Julow found.

Copy link
Collaborator

@Julow Julow 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've sent a patch to your branch but it can wait until the next refactoring: https://github.com/Leonidas-from-XIV/mdx/pull/1

@Leonidas-from-XIV Leonidas-from-XIV merged commit ba41f92 into realworldocaml:main Jan 24, 2022
@Leonidas-from-XIV Leonidas-from-XIV deleted the mli-metadata-labels branch January 24, 2022 15:57
@Leonidas-from-XIV
Copy link
Collaborator Author

We discussed this with @Julow and concluded that it is worth merging, hence I merged it.

Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Jan 28, 2022
CHANGES:

#### Added

- Add support for adding language tags and metadata labels in `mli` files.
  (realworldocaml/mdx#339, realworldocaml/mdx#357, @Julow, @Leonidas-from-XIV)
- Add support for running non-deterministic tests in `dune` MDX 0.2 stanza by
  setting the `MDX_RUN_NON_DETERMINISTIC` environment variable. (realworldocaml/mdx#365,
  realworldocaml/mdx#366, @Leonidas-from-XIV)
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