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

Enable sherlodoc on .odoc-index file for odoc 3 compat #42

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

Conversation

EmileTrotignon
Copy link
Collaborator

This adds support for the future odoc 3, which include generating .odoc-index files for sherlodoc to consume instead of consuming .odocl files directly.

It works with the following odoc PR : ocaml/odoc#1084

index/index.ml Outdated
@@ -55,24 +74,50 @@ let main
close_in h ;
files @ other_files
in
let files = List.map Fpath.v files in
let favourite_files = List.map Fpath.v favourite_files in
Copy link
Owner

Choose a reason for hiding this comment

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

Should we be using Fpath.of_string? (as the doc mentions it's better for untrusted inputs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems to me that of_string return the same error but in a result instead of an exception. Here, if we got an invalid path I think we would crash, so I think v is fine.
We might want to switch to of_string if we wanted to have a warning instead of a crash of invalid paths.

index/index.ml Outdated
let h = Storage.open_out db_filename in
let flush () =
let t = Db_writer.export ~summarize:(db_format = `ancient) db in
Storage.save ~db:h t
in
let loop ~favourite odoc =
let loop kind ~favourite file =
let odoc = Fpath.to_string file in
let pkg, odoc =
match String.split_on_char '\t' odoc with
Copy link
Owner

Choose a reason for hiding this comment

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

It looks very weird that we are parsing a tab-separated format from the odoc variable that was previously treated as a valid filename... Maybe it would be easier to check if the filename is an .odocl or .odoc-index on line 71 below, instead of doing a bunch of List.filter above? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right thats quite weird

@art-w
Copy link
Owner

art-w commented Jun 19, 2024

Would it be possible to add a little test to generate an .odoc-index and a pin to the odoc PR in sherlodoc, such that the CI will run? Thanks! :)

@art-w
Copy link
Owner

art-w commented Sep 5, 2024

The CI seems to report errors with the latest odoc CLI which I'm not familiar with... Any ideas on how to update the cram tests? :)

@EmileTrotignon
Copy link
Collaborator Author

If I understand correctly the CI uses regular odoc, instead of the latest version from master. The corresponding PR in odoc is merged, so a pin on master should fix this.

@EmileTrotignon
Copy link
Collaborator Author

I have updated this to work with the latest odoc
The CI still fails, but only on macos and fails with a weird message : error: RPC failed; curl 92 HTTP/2 stream 5 was not closed cleanly: CANCEL (err 8)

@panglesd panglesd mentioned this pull request Dec 9, 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.

2 participants