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 odoc syntax support #1397

Closed
wants to merge 2 commits into from
Closed

Add odoc syntax support #1397

wants to merge 2 commits into from

Conversation

ryyppy
Copy link
Member

@ryyppy ryyppy commented Oct 4, 2018

This is a tracking branch for #1117

Things which need to be done:

  • Add syntax parameter to documentation stanza
  • Use syntax parameter in the odoc call
  • Add tests to test the right calling behavior

@ryyppy ryyppy requested a review from a user October 4, 2018 14:19
@ryyppy ryyppy force-pushed the add-odoc-syntax-support branch from bf7655e to 86698ed Compare October 5, 2018 09:55
let decode =
enum [ "reason", Reason
; "ocaml", OCaml
; "re", Reason
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that "reason" & "ocaml" are enough. These names are also used in formatting rules for examples.

@@ -1894,20 +1893,34 @@ module Copy_files = struct
end

module Documentation = struct
module Doc_Syntax = struct
type t = Reason | OCaml | All
Copy link
Collaborator

Choose a reason for hiding this comment

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

Other places use something like All | Only of syntax list which is more flexible - with this type you can not only enable a subset, it's all or only one. But this is not a problem since there are only two languages here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I realized that All doesn't make sense anyways. odoc plans to provide both formats within the same html file, so the syntax option is only for the "preferred default syntax" later on

@ryyppy
Copy link
Member Author

ryyppy commented Oct 8, 2018

Currently I am having troubles with the fact that there might be multiple (documentation) stanzas, which could theoretically all have different configurations for the syntax flag.

After some discussion with @rgrinberg on Discord, one way to tackle this is to configure the odoc syntax flag via an env variable instead.

Generally speaking, after ocaml/odoc#129 is resolved, we might only need this flag for selecting a default syntax, but all RE / ML representations should be inlined in the docs and selectable in the UI anyways.

\cc @aantron

@ghost
Copy link

ghost commented Oct 8, 2018

Generally speaking, after ocaml/odoc#129 is resolved, we might only need this flag for selecting a default syntax, but all RE / ML representations should be inlined in the docs and selectable in the UI anyways.

I'm wondering where is the best place to put this parameter. It seems more like a user preference to me, i.e. something that should live in ~/.config/odoc/....

@ryyppy
Copy link
Member Author

ryyppy commented Oct 8, 2018

@diml so my motivation behind this is following UX perspective: I am a Reason library designer and want to use odoc to generate my API, so my target group will most likely be Reason developers who want to see Reason-like syntax first.

If you don't provide any flag, it will default to OCaml. Generating the docs should IMO be reproducible, so I would not like to see user-based configuration mixed in.

There were some concerns that we should also offer a way to expose only one single syntax, so let's wait until ocaml/odoc#129 is resolved.

@emillon
Copy link
Collaborator

emillon commented Oct 8, 2018

I agree that this should be reproductible, similarly to formatting rules not pulling global configuration.

@ghost
Copy link

ghost commented Oct 8, 2018

I see, thanks for the explanation. In general, I assume that the developer will make this choice once for a given project, maybe it should go in the dune-project file then?

@trefis
Copy link
Collaborator

trefis commented Oct 8, 2018

Also, one should note that there has been no release of odoc since v1.2 last year. That is: all the reason support is not on opam yet.
This should be kept in mind before merging things to dune.

@rgrinberg
Copy link
Member

I see, thanks for the explanation. In general, I assume that the developer will make this choice once for a given project, maybe it should go in the dune-project file then?

Hmm, actually I agreed with your original point that it's probably more useful as a user preference for users of the package rather than the developers.

Also, one should note that there has been no release of odoc since v1.2 last year. That is: all the reason support is not on opam yet.

@aantron gave us a warning that there should be a new odoc release this week.

@ghost
Copy link

ghost commented Oct 8, 2018

Indeed, btw I just remembered one question i had about this: if you are using a mix of OCaml and Reason libraries, do you expect to read the doc for OCaml libraries in OCaml syntax and the doc for Reason libraries in Reason syntax?

If yes, then there should indeed be a configuration parameter in dune about this. If not, i.e. you want to read everything in the same syntax, then it feels like a runtime configuration parameter is better suited. Dune doesn't need to know about the latter.

@rgrinberg
Copy link
Member

Closing for staleness. Feel free to re-open once you are ready to make more progress.

@rgrinberg rgrinberg closed this May 1, 2019
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.

4 participants