Support BuckleScript (Backport to 4.02.3) #179

leostera · 2018-09-10T13:13:30Z

In order to support BuckleScript in it's current release, I set up to backport odoc to support the 4.02.x switches. I'm opening this PR as a WIP to gather feedback, ideas, thoughts. Not really a PR to get merged.

The changes aren't too big, just pulling in the result module and making some decision on how to handle the old AST. So far there's only 4 tests that fail — if I read the output of make test correctly 😅

@rizo suggested using migrate-parse-tree, which I'll have a look at later today.

I'm also making a small change in the HTML outputter so it uses regular dashes - instead of short ones ‑ — it's barely perceptible to the naked eye, but a font with ligatures (such as Fira Code) can't apply them otherwise (left is replaced, right is the current output):

I wouldn't go as far as making FiraCode the default font (not everyone likes ligatures) but it should be an easy drop-in.

Sample Generated Docs: ostera/reactor — there's some wonkyness with the links but it's a start.

Anyway! Got a few questions that arose while working on this if anyone feels like chiming in 🤔

Should odoc support BuckleScript at all?
Is maintaining a backport viable?
What would you recommend me to do with the AST migrations? (besides migrate-parse-tree)
How would the Reason-output-generation look like?
Interop with other doc tools? (jsdoc? docusaurus?)

Thanks to the maintainers for this wonderful tool and looking forward to have a single documentation tool that spans OCaml dialects and compilers 🙌

dbuenzli · 2018-09-10T13:31:46Z

I set up to backport odoc to support the 4.02.x switches

IIRC there were quite a few bugs with cmt[i] files before 4.03.0 so supporting 4.02.x is likely to be a maintenance burden for odoc. I would rather be in favor of not supporting this.

leostera · 2018-09-10T15:49:35Z

@dbuenzli I wasn't suggesting that this should be merged 🙈 — if anything I said the opposite:

Not really a PR to get merged.

And I'm more than happy to maintain the backport until BuckleScript moves out of 4.02.x 😄 — I was just looking for some guidance.

Would it be worth publishing under a different name? opam install bs-odoc ?

aantron · 2018-09-10T17:04:52Z

My answers:

Yes, odoc should support BuckleScript.
I don't know the details of the 4.02 cmt bugs, so my opinion should be considered uneducated. I would say we should merge a 4.02 port into master, if it works. My reasoning is:
- BuckleScript will eventually be ported to a higher OCaml version, and then we can delete any problematic 4.02 code from odoc.
- In the meantime, the cmt-related code evolves pretty slowly, so we likely won't have much maintenance to do. If we get too many bug reports related to 4.02, we can give them lower priority, and encourage people to support porting BuckleScript to newer OCaml.
So far, we have been solving AST incompatibilities using cppo (example). ocaml-migrate-parsetree can help with incompatibilities in the Parsetree, but some of the changes you had to do are due to incompatibilities in the Typedtree, so we might have to use cppo for those.
cc @ryyppy for this one :) @ryyppy added Reason syntax support in Add Reason support #156.

How would the Reason-output-generation look like?
This will probably need some refactoring of how output is generated before it can be done.

Regarding Fira Code, we can put it into the font stack in CSS, so it is used if available. I use this font myself, it is nice :) And I agree with emitting markup that fonts will have ligatures for.

My suggestion is to take the Fira Code commit out of this PR, and open a separate PR for it. We can merge that right away.

For the 4.02 port, I suggest using cppo (and maybe ocaml-migrate-parsetree) to fix odoc in this branch for 4.03+. Then, we can merge it.

It may be worthwhile to create a new Compat module, where odoc abstracts away some of the version-specific code. This may not work for all places where we need to do an OCaml version check, though.

At least for String.capitalize, you can do something like this in Compat:

module String =
struct
  include String

  let capitalize s = (capitalize [@ocaml.warning "-3"]) s
end

and open Compat wherever it is needed.

leostera · 2018-09-11T19:08:45Z

@aantron refactored to include a Compat module, and use cppo to work with current OCaml releases too 🙌

Could use some pointers updating the tests too.

aantron · 2018-09-11T20:31:27Z

src/odoc/env.ml

@@ -35,7 +37,7 @@ module Accessible_paths = struct

 let find_file_by_name t name =
 let uname = name ^ ".odoc" in
- let lname = String.uncapitalize_ascii name ^ ".odoc" in
+ let lname = String.uncapitalize name ^ ".odoc" in


Total nit, but I recommend calling these functions in Compat.String by their 4.03 names (uncapitalize_ascii, etc.), so we can later just delete that module and open Compat, and not have to edit the rest of the code.

Makes sense!

aantron · 2018-09-11T20:34:18Z

src/loader/cmi.ml

@@ -524,12 +541,18 @@ let read_label_declaration env parent ld =
 let type_ = read_type_expr env ld.ld_type in
 {id; doc; mutable_; type_}

-let read_constructor_declaration_arguments env parent arg =
+let read_constructor_declaration_arguments env _parent arg =


Looks like _parent should be restored back to parent, since it's used in the 4.03 code. I recommend adding ignore parent; to the 4.02 code to suppress the warning.

aantron · 2018-09-11T20:35:33Z

src/loader/cmti.ml

@@ -160,12 +160,16 @@ let read_label_declaration env parent ld =
 let type_ = read_core_type env ld.ld_type in
 {id; doc; mutable_; type_}

-let read_constructor_declaration_arguments env parent arg =
+let read_constructor_declaration_arguments env _parent arg =


Same thing with _parent, suggest ignore parent; in 4.02 code.

leostera · 2018-09-13T07:32:03Z

@aantron @rizo this is what I came up with: da65fd8#diff-19c336c256d025411f1aa0f1b03f19a2R55

It wasn't really a labelling problem, but rather that the option type seemed to be carried around too. This seems to make all tests happy on 4.02.x and 4.07.x — now awaiting for CI to tell me if it's good on the rest 👌

Happy to refactor as you may see fit so just let me know.

aantron · 2018-09-13T14:06:15Z

LGTM. I have two requests:

Can you add 4.02.3 to Travis, in addition to 4.02.3+buckle-master? This is based on a comment we had in chat, that the two switches generate different ASTs. I suspect that might not actually be the case, but it's better to be sure and remain sure :)
Can you give instructions for how to generate Reason-syntax docs for a BuckleScript project with this PR, i.e. how are you generating your docs?

leostera · 2018-09-13T16:08:53Z

@aantron had to rebase because of the merge-conflict, but added the following changes in new commits for easier review 👍

Also let me know what do you think about the README additions!

aantron · 2018-09-13T19:12:49Z

The README looks good. Thank you!

leostera mentioned this pull request Sep 10, 2018

Use regular dash in arrows to support ligatures #180

Merged

leostera force-pushed the backport/4.02.3 branch 2 times, most recently from e824efb to ef01e14 Compare September 11, 2018 18:59

leostera force-pushed the backport/4.02.3 branch 3 times, most recently from 15d6065 to 65bd138 Compare September 11, 2018 19:23

aantron reviewed Sep 11, 2018

View reviewed changes

leostera force-pushed the backport/4.02.3 branch 7 times, most recently from 330011d to da65fd8 Compare September 13, 2018 07:28

leostera changed the title ~~[WIP] Support BuckleScript (Backport to 4.02.3)~~ Support BuckleScript (Backport to 4.02.3) Sep 13, 2018

leostera added 5 commits September 13, 2018 17:50

Add result dependency

48a156d

Backport to 4.02.x

7a74876

Run 4.02.3+buckle-master on Travis

9e5ea6a

Add compat module for older releases

4d8accc

Fix broken tests for 4.02.x

969bcde

leostera force-pushed the backport/4.02.3 branch from da65fd8 to 969bcde Compare September 13, 2018 15:51

leostera added 2 commits September 13, 2018 17:51

Run CI also on vanilla 4.02.3

773a686

Add BuckleScript usage in README

cb28cdf

aantron merged commit e1e73ba into ocaml:master Sep 13, 2018

leostera deleted the backport/4.02.3 branch September 13, 2018 19:13

aantron mentioned this pull request Oct 16, 2018

odoc 1.3.0: OCaml documentation generator ocaml/opam-repository#12811

Merged

dbuenzli mentioned this pull request Feb 7, 2019

No more GADTs #292

Merged

dbuenzli mentioned this pull request Jul 23, 2020

Improve xref2 tests #451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support BuckleScript (Backport to 4.02.3) #179

Support BuckleScript (Backport to 4.02.3) #179

leostera commented Sep 10, 2018 •

edited

Loading

dbuenzli commented Sep 10, 2018

leostera commented Sep 10, 2018

aantron commented Sep 10, 2018

leostera commented Sep 11, 2018

aantron Sep 11, 2018

leostera Sep 11, 2018

aantron Sep 11, 2018

leostera Sep 11, 2018

aantron Sep 11, 2018

leostera Sep 11, 2018

leostera commented Sep 13, 2018

aantron commented Sep 13, 2018

leostera commented Sep 13, 2018

aantron commented Sep 13, 2018

Support BuckleScript (Backport to 4.02.3) #179

Support BuckleScript (Backport to 4.02.3) #179

Conversation

leostera commented Sep 10, 2018 • edited Loading

dbuenzli commented Sep 10, 2018

leostera commented Sep 10, 2018

aantron commented Sep 10, 2018

leostera commented Sep 11, 2018

aantron Sep 11, 2018

Choose a reason for hiding this comment

leostera Sep 11, 2018

Choose a reason for hiding this comment

aantron Sep 11, 2018

Choose a reason for hiding this comment

leostera Sep 11, 2018

Choose a reason for hiding this comment

aantron Sep 11, 2018

Choose a reason for hiding this comment

leostera Sep 11, 2018

Choose a reason for hiding this comment

leostera commented Sep 13, 2018

aantron commented Sep 13, 2018

leostera commented Sep 13, 2018

aantron commented Sep 13, 2018

leostera commented Sep 10, 2018 •

edited

Loading