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

Ast_builder documentation #518

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

patricoferris
Copy link
Collaborator

Related to: #324, #435

Much of Ast_builder is undocumented. The names of functions are derived from the things they construct from Ast.ml (mostly AST nodes from Parsetree). The parsetree is well-documented. This PR pulls through that documentation directly to Ast_builder.

In order for this to work well, the gen_ast_builder script now produces module types for the interface to Ast_builder (one for functions with ~loc arguments called S_located and one without, corresponding to the original S).

Before:

Screenshot 2024-08-30 at 14 53 03

After:

Screenshot 2024-08-30 at 14 49 12

Known problems:

Some of the doc attributes pulled from the parsetree contain odoc references which clash with the function names defined in the generated code e.g. type label_declaration and val label_declaration clash for the documentation comment attached to ptyp_poly.


If there is agreement to add this we could do the same for Ast_pattern I'm sure.

Copy link
Collaborator

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

This looks great! Awesome addition!

I have a few minor code comments but other than that I think it's a great idea and a very useful addition to the documentation.

On a slightly unrelated note, I wonder if we shouldn't change the way we generate those files to promote the generated sources so they can be inspected and we are notified whenever they are updated rather than all of it being silent as it is now. That's of course a matter for a separate PR but I'm still curious to know what you think about it!

src/gen/gen_ast_builder.ml Outdated Show resolved Hide resolved
src/gen/gen_ast_builder.ml Show resolved Hide resolved
src/gen/gen_ast_builder.ml Outdated Show resolved Hide resolved
src/gen/gen_ast_builder.ml Outdated Show resolved Hide resolved
src/gen/gen_ast_builder.ml Show resolved Hide resolved
@patricoferris
Copy link
Collaborator Author

On a slightly unrelated note, I wonder if we shouldn't change the way we generate those files to promote the generated sources so they can be inspected and we are notified whenever they are updated rather than all of it being silent as it is now. That's of course a matter for a separate PR but I'm still curious to know what you think about it!

I think that's a great idea and would make debugging a little easier I think (a few times, especially for docs, I was having to open _build/default/src/ast_builder_generated.ml

Copy link
Collaborator

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's follow up on promoting the generated files and optimizing the parse/print loops in a few places!

I'll open issues for both!

@NathanReb
Copy link
Collaborator

@patricoferris I think you can go ahead and merge this!

patricoferris and others added 7 commits September 26, 2024 15:21
Signed-off-by: Patrick Ferris <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>

Co-authored-by: Nathan Rebours <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>

Co-authored-by: Nathan Rebours <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>

Co-authored-by: Nathan Rebours <[email protected]>
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