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

New driving page #1209

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

New driving page #1209

wants to merge 6 commits into from

Conversation

panglesd
Copy link
Collaborator

@panglesd panglesd commented Oct 8, 2024

This new driving page contains everything one should know to write an odoc 3.0 driver, including to drive doc generation for opam-installed package.

Unfortunately, I feel the style is not great... And I'm not sure everything is clear... but at least the structure of the explanation is there.

It explains the current status of the CLI (assuming #1206 is merged), and should be updated when new changes are added.

It is also a good time for the community to give feedback!

@panglesd
Copy link
Collaborator Author

panglesd commented Oct 8, 2024

The page can be consulted rendered here. As always I'll try to keep it up to date, but no guarantee.

Copy link
Contributor

@lukemaurer lukemaurer left a comment

Choose a reason for hiding this comment

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

This is great, thanks. I just found a few typos and have a couple of clarifying questions

doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated
Comment on lines 48 to 49
package. Any of its libraries will have an associated "module unit", with the
name of the library. Another package can refer to the doc using the package
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't quite clear here what the module unit does? Is it the page for the module that is the library itself? Is a submodule not also a module unit? If it isn't, might we call module units library units instead (since only whole libraries have them)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review!

I named it "module unit" since it contains modules, just as a "page unit" contains page.

However, the "unit" name is very confusing I think, and was a bad choice, since we already have "OCaml compilation units" for .cm{t;ti;i} and sometimes "odoc units" for .odoc.

I'll update that to hopefully something less confusing!

doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated
- [-L <name>:<dir>] are used to list the "named module-units", used to resolve
references such as [{!/findlib.dynload/Fl_dynload}].

- [-I <dir>] are used to resolve non-references, and should include the same set
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what a non-reference is? Especially if it's still something that can be resolved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is indeed confusing. I'm not even sure what I wanted to say...

@panglesd panglesd added the no changelog This pull request does not need a changelog entry label Oct 17, 2024
@panglesd
Copy link
Collaborator Author

panglesd commented Oct 17, 2024

I hope I clarified the confusion with the introduction of a new name for the -P and -L groups: "page trees" and "module trees", that are named and rooted.

@jonludlam
Copy link
Member

Given we've just had a chat about not using submodules for sherlodoc, can you back out those changes please?

@panglesd
Copy link
Collaborator Author

Hmm, sorry I'm not sure to understand which change you are talking about! (wrong PR maybe?)

.gitmodules Outdated
[submodule "sherlodoc"]
path = sherlodoc
url = https://github.com/Julow/sherlodoc
branch = odoc3_compat
Copy link
Member

Choose a reason for hiding this comment

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

These changes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yes sorry, I accidentally pushed them when pushing the last commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR branch now only contains the relevant commits. If you want to build the odoc doc with dune exec -- odoc_driver -p odoc, then use the new_driving_guide_work branch.

Copy link
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

I spotted a few typos and I have a few questions, but that's a very useful write-up.

doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated
other trees, a [-L foo:<odoc_dir>/foo/lib/foo] argument needs to be added
at the link phase.

- A module tree named [foo.bar], with root id [foo/doc]. When referred from
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the ID correct here?

doc/driver.mld Outdated Show resolved Hide resolved
doc/driver.mld Outdated Show resolved Hide resolved
@panglesd
Copy link
Collaborator Author

panglesd commented Nov 4, 2024

Thanks @gpetiot for the careful review!

This documentation is likely to require some more updates, as we change the "canonical hierarchy" that we describe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This pull request does not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants