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

Separate commands for html generation of page+units and implementation #1188

Merged
merged 12 commits into from
Aug 22, 2024

Conversation

panglesd
Copy link
Collaborator

This was suggested in #1185 (comment)

The benefit is cleaner man pages, safer CLI validation.

This PR also takes this opportunity to do some cleanup, by removing the old and never used way of rendering sources:

  • Remove the "source dir" identifier. We have enough identifier types!
  • Remove the source_tree odoc unit.
  • Remove the --source-root option which is not anymore usable in odoc 3.

Once this is merged, I'll rebase #1185 on top and apply @Julow's comment!

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Awesome! All this code that we didn't need is gone :)

@@ -1,11 +0,0 @@
open Or_error

val compile :
Copy link
Collaborator

Choose a reason for hiding this comment

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

The driver currently did not build source trees but this change means that the driver will have to generate regular pages in the source hierarchy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the odoc source tree mechanism is not well suited for generating source pages in the context of odoc 3. If we design a way for odoc to generate the missing pages, it would be different from the one removed here. So I think it is safe to remove it!

@jonludlam
Copy link
Member

Ooh I do like a good cleanup :-)

@jonludlam
Copy link
Member

Just want to check though: should it be odoc html-generate-source or src rather than odoc html-generate-impl? Asking for opinions, I don't have a strong opinion.

@jonludlam
Copy link
Member

Also we should have some negative tests - what happens when you do odoc html-generate on an impl file and vice-versa.

@panglesd
Copy link
Collaborator Author

panglesd commented Aug 21, 2024

In the odoc dev meeting, we decided to rename it to html-generate-source and use the source file as positional argument while the implementation file is passed as an option. It'll make it easier when including dune files or other file in source rendering.

Source is now the positional argument, while impl is passed as an argument. When
we generate source for other files (such as dune or .mli or Makefiles or ...)
this will come handy.
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

I think this is ready to be merged :) I've left some comments but I think it's fine to discuss them after this is merged.

CHANGES.md Outdated Show resolved Hide resolved
src/odoc/bin/main.ml Outdated Show resolved Hide resolved
src/odoc/bin/main.ml Outdated Show resolved Hide resolved
src/odoc/bin/main.ml Show resolved Hide resolved
src/odoc/rendering.ml Outdated Show resolved Hide resolved
@panglesd panglesd merged commit 3e332c9 into ocaml:master Aug 22, 2024
7 of 12 checks passed
panglesd added a commit that referenced this pull request Aug 22, 2024
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