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

Add Reason support #156

Merged
merged 22 commits into from Aug 8, 2018

Conversation

Projects
None yet
5 participants
@ryyppy
Copy link
Contributor

commented Jul 9, 2018

Tracking Branch for #129

Things that are being worked on:

  • Add --lang flag for html command (currently not building b/c of type error)
  • Copy test cases to reason syntax & implement as much Reason syntax generation as possible
  • Apply refmt on code snippets inside block comments
  • Use location info to determine ml or re formatting for Code_blocks
  • Provide mechanics to configure refmt's heuristic constructor list (prevent explicit_arity attributes) -> use ODOC_REFMT_HEURISTICS env variable

Test Cases:

  • Val
  • Markup
  • Section
  • Module
  • Interlude
  • Include
  • Mld
  • Type
  • External
  • Functor
  • Class
  • Stop

ryyppy added some commits Jul 9, 2018

Add --lang flag support
The semantics of this flag are subject of change though.
Add refmt transform for nested block elements
This commit introduces following changes:
- Move language Variant from Html_page to Html_tree
- Introduce a `~lang` parameter for the Documentation module
- Manipulate block level code snippets by applying refmt
- Attach a classname `ml` / `re` for each translated code snippet
- If a codesnippet could not be translated, keep the OCaml representation
Detect source lang & compare with target lang
This adds language detection by checking the file extension of the
compilation unit. Depending on the target language (~to_lang), it
will do the proper Reason -> OCaml or OCaml -> Reason transformation
in `Code_block`s. In case of to_lang = from_lang, no code
transformation will be applied.
@ryyppy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

The most intrusive changes are in the src/html/documentation.ml module.
I introduced a ~from_lang / ~to_lang parameter for block_elements which give context on which parser / printer logic to use. Rest of the work is dedicated to the signature markup rendering part.

module Type : sig
val path :
[< Html_types.span_content_fun ] Html.elt list ->
module ML : sig

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

I introduced namespaces for Markup... common Markup is still in the toplevel of the module, e.g. keyword

end

val arrow : [> Html_types.span ] Html.elt
(** "->" with a non breaking hyphen, styled as a keyword. *)
module RE : sig

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

It's probably worth introducing a common module interface to make the signature more DRY

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

That seems like a good idea, but we can do it later, during some future refactoring/cleanup.

@@ -125,25 +125,25 @@ struct
-> Model.Lang.TypeExpr.t -> ('inner, 'outer) text Html.elt list
= fun ?(needs_parentheses=false) t ->
match t with
| Var s -> [Markup.Type.var ("'" ^ s)]
| Any -> [Markup.Type.var "_"]
| Var s -> [Markup.ML.Type.var ("'" ^ s)]

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

I guess this kind of pattern can be solved with first class modules as well? dropping the namespace and just reify a specific module implementation for each language? This solution was quicker and the least intrusive

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

Is this comment in reference to similarity between this code, and the corresponding function in to_re_html_tree.ml? If so, yes, if many of the functions are similar, it might be worthwhile to factor the common parts out, and use either first-class modules or a functor. As with most other things, we can save that for a later refactoring, though :)

It could, however, make the diff much smaller, and, so, review much easier. I did a diff between to_html_tree.ml and to_re_html_tree.re to help the review, and the diff was pretty small, so a lot of code seems to be repeated.

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 8, 2018

Author Contributor

I don't know how you would factor this out... I was thinking of using include statements, but most bindings of this module are recursively anded... not sure if you can override specific functions then?

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

IIRC, in the diff between to_html_tree.ml and to_re_html_tree.ml, the skeleton of the functions is the same, and only some details change (what delimiter to use in records, variants, whether there is a semicolon, etc). So, we would factor out these details into ML and Re modules, and turn To_html_tree into a functor that contains the remaining common code.

* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*)

val compilation_unit : ?theme_uri:Html_tree.uri -> Model.Lang.Compilation_unit.t -> Html_tree.t

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

This should probably just reuse the module definition of to_html_tree.mli... I was not too sure how to do this here, also I thought I would let this be for now and redesign this in a more unified way

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

You could do include module type of To_html_tree, I believe, but it is indeed better to wait to unify.

@@ -10,3 +10,25 @@ let rec list_concat_map ?sep ~f = function
match sep with
| None -> hd @ tl
| Some sep -> hd @ sep :: tl

(**

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

I left this code in here, since there were some discussions on how to introduce refmt without coupling it to the odoc core

@@ -20,6 +20,9 @@ let at : span -> 'a -> 'a with_location = fun location value ->
let location : 'a with_location -> span = fun {location; _} ->
location

let file_ext : span -> string = fun span ->

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

Required to be able to determine the from_lang attribute... if .mli -> OCaml, if .rei -> Reason

@@ -19,8 +19,14 @@
module Html = Tyxml.Html
module Paths = Model.Paths

type lang = OCaml | Reason

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

I introduced the language type in here, not sure if this is the right module

let from_odoc ~env ?theme_uri ~output:root_dir input =
let to_html_tree_page ?theme_uri ~lang v =
match lang with
| Html.Html_tree.Reason -> Html.To_html_tree.RE.page ?theme_uri v

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

I don't like this... I felt like we should use something like first-class-modules, but I was not entirely sure how to do this the right way... feedback welcome!

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

There may be som nice solution with first-class modules, but I suspect it's not worth looking for, for something so simple as this match expression :)

| Html.Html_tree.Reason -> Html.To_html_tree.RE.compilation_unit ?theme_uri v
| Html.Html_tree.OCaml -> Html.To_html_tree.ML.compilation_unit ?theme_uri v

let from_odoc ~env ?(lang=Html.Html_tree.OCaml) ?theme_uri ~output:root_dir input =

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 7, 2018

Author Contributor

The CLI has an optional lang parameter, which defaults to OCaml

@aantron aantron self-requested a review Aug 7, 2018

@aantron
Copy link
Contributor

left a comment

Looks good, thanks! I'm happy to merge this PR after a brief discussion, and we can finish off things here or in additional PRs.

  • I think module declarations, module type declarations, and type declarations are missing the final ; (see e.g. module.html in the HTML expect tests).
  • The most dubious thing in the PR is, I think, the duplication of most of the code in to_html_tree.ml into to_re_html_tree.ml. But we can figure out how to factor the common code out in a future PR or in general cleanup.
  • How should the new --lang argument be integrated with build systems?
end

val arrow : [> Html_types.span ] Html.elt
(** "->" with a non breaking hyphen, styled as a keyword. *)
module RE : sig

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

That seems like a good idea, but we can do it later, during some future refactoring/cleanup.

@@ -125,25 +125,25 @@ struct
-> Model.Lang.TypeExpr.t -> ('inner, 'outer) text Html.elt list
= fun ?(needs_parentheses=false) t ->
match t with
| Var s -> [Markup.Type.var ("'" ^ s)]
| Any -> [Markup.Type.var "_"]
| Var s -> [Markup.ML.Type.var ("'" ^ s)]

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

Is this comment in reference to similarity between this code, and the corresponding function in to_re_html_tree.ml? If so, yes, if many of the functions are similar, it might be worthwhile to factor the common parts out, and use either first-class modules or a functor. As with most other things, we can save that for a later refactoring, though :)

It could, however, make the diff much smaller, and, so, review much easier. I did a diff between to_html_tree.ml and to_re_html_tree.re to help the review, and the diff was pretty small, so a lot of code seems to be repeated.

* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*)

val compilation_unit : ?theme_uri:Html_tree.uri -> Model.Lang.Compilation_unit.t -> Html_tree.t

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

You could do include module type of To_html_tree, I believe, but it is indeed better to wait to unify.

let from_odoc ~env ?theme_uri ~output:root_dir input =
let to_html_tree_page ?theme_uri ~lang v =
match lang with
| Html.Html_tree.Reason -> Html.To_html_tree.RE.page ?theme_uri v

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

There may be som nice solution with first-class modules, but I suspect it's not worth looking for, for something so simple as this match expression :)

@@ -1,5 +1,5 @@
<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml"><head><title>Markup (test_package.Markup)</title><link rel="stylesheet" href="../../odoc.css"/><meta charset="utf-8"/><meta name="viewport" content="width=device-width,initial-scale=1.0"/><script src="../../highlight.pack.js"></script><script>hljs.initHighlightingOnLoad();</script></head><body><div class="content"><header><nav><a href="../index.html">Up</a> – <a href="../index.html">test_package</a> &#x00BB; Markup</nav><h1>Module <code>Markup</code></h1><p>Here, we test the rendering of comment markup.</p><nav class="toc"><ul><li><a href="#sections">Sections</a><ul><li><a href="#subsection-headings">Subsection headings</a><ul><li><a href="#sub-subsection-headings">Sub-subsection headings</a></li><li><a href="#anchors">Anchors</a></li></ul></li></ul></li><li><a href="#styling">Styling</a></li><li><a href="#links-and-references">Links and references</a></li><li><a href="#preformatted-text">Preformatted text</a></li><li><a href="#lists">Lists</a></li><li><a href="#unicode">Unicode</a></li><li><a href="#raw-html">Raw HTML</a></li><li><a href="#tags">Tags</a></li></ul></nav></header><section><header><h2 id="sections"><a href="#sections" class="anchor"></a>Sections</h2><p>Let's get these done first, because sections will be used to break up the rest of this test.</p><p>Besides the section heading above, there are also</p></header><section><header><h3 id="subsection-headings"><a href="#subsection-headings" class="anchor"></a>Subsection headings</h3><p>and</p></header><section><header><h4 id="sub-subsection-headings"><a href="#sub-subsection-headings" class="anchor"></a>Sub-subsection headings</h4><p>but odoc has banned deeper headings. There are also title headings, but they are only allowed in mld files.</p></header></section><section><header><h4 id="anchors"><a href="#anchors" class="anchor"></a>Anchors</h4><p>Sections can have attached <a href="index.html#anchors"><span>Anchors</span></a>, and it is possible to <a href="index.html#anchors"><span>link</span></a> to them. Links to section headers should not be set in source code style.</p></header></section></section></section><section><header><h2 id="styling"><a href="#styling" class="anchor"></a>Styling</h2><p>This paragraph has some styled elements: <b>bold</b> and <i>italic</i>, <b><i>bold italic</i></b>, <em>emphasis</em>, <em><em>emphasis</em> within emphasis</em>, <b><i>bold italic</i></b>, super<sup>script</sup>, sub<sub>script</sub>. The line spacing should be enough for superscripts and subscripts not to look odd.</p><p><code>code</code> is a different kind of markup that doesn't allow nested markup.</p><p>It's possible for two markup elements to appear <b>next</b> <i>to</i> each other and have a space, and appear <b>next</b><i>to</i> each other with no space. It doesn't matter <b>how</b> <i>much</i> space it was in the source: in this sentence, it was two space characters. And in this one, there is <b>a</b> <i>newline</i>.</p><p>Code can appear <b>inside <code>other</code> markup</b>. Its display shouldn't be affected.</p></header></section><section><header><h2 id="links-and-references"><a href="#links-and-references" class="anchor"></a>Links and references</h2><p>This is a <a href="#">link</a>. It sends you to the top of this page. Links can have markup inside them: <a href="#"><b>bold</b></a>, <a href="#"><i>italics</i></a>, <a href="#"><em>emphasis</em></a>, <a href="#">super<sup>script</sup></a>, <a href="#">sub<sub>script</sub></a>, and <a href="#"><code>code</code></a>. Links can also be nested <em><a href="#">inside</a></em> markup. Links cannot be nested inside each other. This link has no replacement text: <a href="#">#</a>. The text is filled in by odoc. This is a shorthand link: <a href="#">#</a>. The text is also filled in by odoc in this case.</p><p>This is a reference to <a href="index.html#val-foo"><code>foo</code></a>. References can have replacement text: <a href="index.html#val-foo"><span>the value foo</span></a>. Except for the special lookup support, references are pretty much just like links. The replacement text can have nested styles: <a href="index.html#val-foo"><span><b>bold</b></span></a>, <a href="index.html#val-foo"><span><i>italic</i></span></a>, <a href="index.html#val-foo"><span><em>emphasis</em></span></a>, <a href="index.html#val-foo"><span>super<sup>script</sup></span></a>, <a href="index.html#val-foo"><span>sub<sub>script</sub></span></a>, and <a href="index.html#val-foo"><span><code>code</code></span></a>. It's also possible to surround a reference in a style: <b><a href="index.html#val-foo"><code>foo</code></a></b>. References can't be nested inside references, and links and references can't be nested inside each other.</p></header></section><section><header><h2 id="preformatted-text"><a href="#preformatted-text" class="anchor"></a>Preformatted text</h2><p>This is a code block:</p><pre><code>let foo = ()
<html xmlns="http://www.w3.org/1999/xhtml"><head><title>Markup (test_package.Markup)</title><link rel="stylesheet" href="../../odoc.css"/><meta charset="utf-8"/><meta name="viewport" content="width=device-width,initial-scale=1.0"/><script src="../../highlight.pack.js"></script><script>hljs.initHighlightingOnLoad();</script></head><body><div class="content"><header><nav><a href="../index.html">Up</a> – <a href="../index.html">test_package</a> &#x00BB; Markup</nav><h1>Module <code>Markup</code></h1><p>Here, we test the rendering of comment markup.</p><nav class="toc"><ul><li><a href="#sections">Sections</a><ul><li><a href="#subsection-headings">Subsection headings</a><ul><li><a href="#sub-subsection-headings">Sub-subsection headings</a></li><li><a href="#anchors">Anchors</a></li></ul></li></ul></li><li><a href="#styling">Styling</a></li><li><a href="#links-and-references">Links and references</a></li><li><a href="#preformatted-text">Preformatted text</a></li><li><a href="#lists">Lists</a></li><li><a href="#unicode">Unicode</a></li><li><a href="#raw-html">Raw HTML</a></li><li><a href="#tags">Tags</a></li></ul></nav></header><section><header><h2 id="sections"><a href="#sections" class="anchor"></a>Sections</h2><p>Let's get these done first, because sections will be used to break up the rest of this test.</p><p>Besides the section heading above, there are also</p></header><section><header><h3 id="subsection-headings"><a href="#subsection-headings" class="anchor"></a>Subsection headings</h3><p>and</p></header><section><header><h4 id="sub-subsection-headings"><a href="#sub-subsection-headings" class="anchor"></a>Sub-subsection headings</h4><p>but odoc has banned deeper headings. There are also title headings, but they are only allowed in mld files.</p></header></section><section><header><h4 id="anchors"><a href="#anchors" class="anchor"></a>Anchors</h4><p>Sections can have attached <a href="index.html#anchors"><span>Anchors</span></a>, and it is possible to <a href="index.html#anchors"><span>link</span></a> to them. Links to section headers should not be set in source code style.</p></header></section></section></section><section><header><h2 id="styling"><a href="#styling" class="anchor"></a>Styling</h2><p>This paragraph has some styled elements: <b>bold</b> and <i>italic</i>, <b><i>bold italic</i></b>, <em>emphasis</em>, <em><em>emphasis</em> within emphasis</em>, <b><i>bold italic</i></b>, super<sup>script</sup>, sub<sub>script</sub>. The line spacing should be enough for superscripts and subscripts not to look odd.</p><p><code>code</code> is a different kind of markup that doesn't allow nested markup.</p><p>It's possible for two markup elements to appear <b>next</b> <i>to</i> each other and have a space, and appear <b>next</b><i>to</i> each other with no space. It doesn't matter <b>how</b> <i>much</i> space it was in the source: in this sentence, it was two space characters. And in this one, there is <b>a</b> <i>newline</i>.</p><p>Code can appear <b>inside <code>other</code> markup</b>. Its display shouldn't be affected.</p></header></section><section><header><h2 id="links-and-references"><a href="#links-and-references" class="anchor"></a>Links and references</h2><p>This is a <a href="#">link</a>. It sends you to the top of this page. Links can have markup inside them: <a href="#"><b>bold</b></a>, <a href="#"><i>italics</i></a>, <a href="#"><em>emphasis</em></a>, <a href="#">super<sup>script</sup></a>, <a href="#">sub<sub>script</sub></a>, and <a href="#"><code>code</code></a>. Links can also be nested <em><a href="#">inside</a></em> markup. Links cannot be nested inside each other. This link has no replacement text: <a href="#">#</a>. The text is filled in by odoc. This is a shorthand link: <a href="#">#</a>. The text is also filled in by odoc in this case.</p><p>This is a reference to <a href="index.html#val-foo"><code>foo</code></a>. References can have replacement text: <a href="index.html#val-foo"><span>the value foo</span></a>. Except for the special lookup support, references are pretty much just like links. The replacement text can have nested styles: <a href="index.html#val-foo"><span><b>bold</b></span></a>, <a href="index.html#val-foo"><span><i>italic</i></span></a>, <a href="index.html#val-foo"><span><em>emphasis</em></span></a>, <a href="index.html#val-foo"><span>super<sup>script</sup></span></a>, <a href="index.html#val-foo"><span>sub<sub>script</sub></span></a>, and <a href="index.html#val-foo"><span><code>code</code></span></a>. It's also possible to surround a reference in a style: <b><a href="index.html#val-foo"><code>foo</code></a></b>. References can't be nested inside references, and links and references can't be nested inside each other.</p></header></section><section><header><h2 id="preformatted-text"><a href="#preformatted-text" class="anchor"></a>Preformatted text</h2><p>This is a code block:</p><pre><code class="ml">let foo = ()

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

Do you know offhand what changed in this test?

This comment has been minimized.

Copy link
@ryyppy

ryyppy Aug 8, 2018

Author Contributor

I introduced a class attribute for each pre block (see https://github.com/ocaml/odoc/pull/156/files#diff-41083926a76829c938b1f88df53c48d7R274)

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

Thanks, I had seen that but not made the connection. I gave myself a TODO to get these expected strings indented nicely, so that the diffs are actually helpful.

@@ -3,4 +3,4 @@
(library
((name html)
(preprocess (pps (bisect_ppx -conditional)))
(libraries (model tyxml))))
(libraries (model tyxml reason))))

This comment has been minimized.

Copy link
@aantron

aantron Aug 8, 2018

Contributor

Do we still have a dep on reason here? I think the code that uses reason is commented out, but not sure if I missed some.

aantron and others added some commits Aug 8, 2018

Travis: pin Dune to 1.0.0
Dune 1.1.0 doesn't support the --dev flag, which is part of the odoc
build for now. See

  https://travis-ci.org/ocaml/odoc/jobs/413265420#L793

ryyppy added some commits Aug 8, 2018

@aantron

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

Looks like the remaining problem is a failure on 4.03, because Filename.extension was only added in 4.04. Since odoc depends on fpath, we can probably deal with this by replacing Filename.extension with some combination of these functions from fpath.

@aantron

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

You could also use Filename.check_suffix instead of extracting the extension.

ryyppy added some commits Aug 8, 2018

@aantron aantron merged commit c71e87b into ocaml:master Aug 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

aantron added a commit that referenced this pull request Aug 8, 2018

@aantron

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

@diml, @rgrinberg, We'd like to be able to choose, through Dune, whether odoc outputs OCaml syntax, or Reason syntax. This is controlled by passing --lang=re or --lang=ml to odoc html (--lang=ml is the default). What do you recommend for achieving this?

In the future, we might want to generate both outputs, and give users the option to switch between them. I guess Reason output wouldn't require support from Dune at that point.

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

@aantron you should open a dune ticket to discuss this. There's a couple of options here. The most natural one is to create separate targets for reason/ocaml documentation and then use 2 aliases to control them. For example, @doc and @reason-doc. Would you like to take a stab at implementing this in dune?

@diml

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

What about the following simple rule: if the original source file is a .mli, output in OCaml syntax, if it is a .rei, output in reason syntax?

@aantron aantron referenced this pull request Sep 25, 2018

Merged

Refactor HTML Generation #201

2 of 3 tasks complete

@ryyppy ryyppy changed the title Add Reason support (WIP) Add Reason support Jan 19, 2019

@jordwalke

This comment has been minimized.

Copy link

commented Feb 3, 2019

@diml That would work, except there are reasons to have .mli files which generate Reason docs (or vice versa). Many people building Reason apps want to try out opam libraries and the more we can do to make that natural, the more exposure the opam libraries would receive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.