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

Expose parser internals and add Parser.parse_comment_raw #351

Merged
merged 1 commit into from
Jun 17, 2019

Conversation

Julow
Copy link
Collaborator

@Julow Julow commented Apr 16, 2019

This PR exposes the internal modules of the parser.

I figured that Parser_.Ast is a bit simpler than Model.Comment and a lot simpler to obtain.
It also contains more informations (eg. heading labels)

I also add Parser.parse_comment_raw that returns an Ast.docs.

@aantron
Copy link
Contributor

aantron commented Apr 25, 2019

a lot simpler to obtain

What would make Model.Comment easier for you to obtain?

It also contains more informations (eg. heading labels)

I suggest to retain any information you need in Model.Comment, rather than exposing another data representation.

@Julow
Copy link
Collaborator Author

Julow commented Apr 26, 2019

By simpler I meant it's no longer needed to construct the containing_definition argument with dummy values.
I agree if it was only for heading labels, changing Model.Comment would be better. But I was hoping to continue simplifying the parser in later PR. In particular using a simpler representation for references in Parser.Ast.

I'm using Odoc in OCamlformat to reformat documentation comments. Currently, Odoc is quite hard to use and I'd like to improve this.

@jonludlam
Copy link
Member

jonludlam commented May 1, 2019

Ah, having to build a dummy argument to the function is definitely something we should fix.

I'm not quite sure I follow the argument about exposing Ast over Model.Comment - the heading labels are still there, aren't they? (admittedly embedded in an Identifier, so not quite so conveniently). I couldn't see anything else missing either.

@Julow
Copy link
Collaborator Author

Julow commented May 1, 2019

The problem with heading labels is that they are not optional in Model.Comment and it's impossible to tell if it's present in the source or if it's a default value.
Also, I think an even simpler AST would be useful for user of the libraries that just want to parse comments.
For example, in ocaml-ppx/ocamlformat#721, I had to fight the AST to print references and a lot of comments are not formatted because of warnings that could be delayed until the semantics pass.

@jonludlam
Copy link
Member

We could put the original text in the reference tag:

`Reference of Reference.t * string * link_content

and mark the headers with whether the label was generated or specified?

It doesn't seem totally unreasonable to me that ocamlformat only formats valid ocamldoc markup, to be honest, so long as we can surface the warnings to the user. It's a bit of an incentive to write the docs correctly :-)

This was referenced May 3, 2019
@aantron
Copy link
Contributor

aantron commented Jun 11, 2019

Closing this PR, as I believe the discussion in #355 supersedes it.

@aantron aantron closed this Jun 11, 2019
@Julow Julow mentioned this pull request Jun 13, 2019
@Julow
Copy link
Collaborator Author

Julow commented Jun 17, 2019

Thanks for the reopen !

I rebased on master and changed the way the AST is exposed.

@aantron aantron merged commit 21d82a2 into ocaml:master Jun 17, 2019
@aantron
Copy link
Contributor

aantron commented Jun 17, 2019

Looks good. Thanks for your patience and initiative!

@Julow
Copy link
Collaborator Author

Julow commented Jun 17, 2019

Thanks !

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.

3 participants