Rudimentary support for assets #975

trefis · 2023-06-22T15:54:05Z

Context: #59 , #972

No support for referring to assets yet.
This only provides a way to get odoc to place them "in the right place" in the directory structure; so it's only marginally useful.

Current interface:

children given to odoc compile can now be of the form asset-filename
html-generate and html-targets commands now accept --asset FILE arguments, which are paired (based on filename) with the relevant child asset. Warnings are emitted for unknown and missing assets.
files passed to html-generate through --asset are copied in the directory of their parent; would it be better to put them in an _assets subdirectory?

This is illustrated in test/pages/assets.t/run.t

trefis · 2023-06-22T15:55:23Z

src/document/url.ml

+    | { iv = `AssetFile (p, _name); _ } ->
+        let page = Path.from_identifier p in
+        Ok { page; kind = `File; anchor = "" }


I copied what was done for `SourcePage here, but honestly it looks weird.

Is it the anchor = "" which is bothering you?

For some reason, "no anchor" is represented as "empty anchor". The type Url.t is even defined as Anchor.t. See any identifier which is a page, and thus has no anchor (Page, LeafPage, Root, ...)

So that's correct, but I agree it would look better if the anchor field was an option.

Is it the anchor = "" which is bothering you?

No, it's that we're getting a url to the parent of the asset, not the asset itself.

Indeed, that's a bug, thanks for spotting it! It was not harmful because we only call Path.from_identifier on the `SourcePage variant, never Anchor.from_identifier.

Suggested change

| { iv = `AssetFile (p, _name); _ } ->

let page = Path.from_identifier p in

Ok { page; kind = `File; anchor = "" }

| { iv = `AssetFile _; _ } as p ->

let page = Path.from_identifier p in

Ok { page; kind = `File; anchor = "" }

Could you correct SourcePage and SourceDir as well?

src/document/url.mli

src/model/paths.ml

trefis · 2023-06-22T15:58:06Z

test/pages/assets.t/run.t

+  File "img.jpg":
+  Warning: asset is missing.


I wondered if this should have been a fatal error, since I didn't know how to make it be one I managed to convince myself that it shouldn't be.

I think odoc tries to avoid failing too badly in case of errors.

So I don't think it should be a hard failure, but that should not stop processing the other assets (see above)

panglesd

Nice!
I'm not sure why assets could not be children of compilation unit. But that should not be a too restrictive restriction, so I'm happy with it!
The only thing I would like to see changed is the non-handling of other assets in case of a missing asset.

src/document/url.mli

panglesd · 2023-06-23T13:59:53Z

src/document/url.ml

+    | { iv = `AssetFile (p, _name); _ } ->
+        let page = Path.from_identifier p in
+        Ok { page; kind = `File; anchor = "" }


Is it the anchor = "" which is bothering you?

For some reason, "no anchor" is represented as "empty anchor". The type Url.t is even defined as Anchor.t. See any identifier which is a page, and thus has no anchor (Page, LeafPage, Root, ...)

So that's correct, but I agree it would look better if the anchor field was an option.

src/model/paths.ml

src/model_desc/paths_desc.ml

src/odoc/html_page.ml

panglesd · 2023-06-23T14:21:42Z

test/pages/assets.t/run.t

+  File "img.jpg":
+  Warning: asset is missing.


I think odoc tries to avoid failing too badly in case of errors.

So I don't think it should be a hard failure, but that should not stop processing the other assets (see above)

panglesd · 2023-06-23T14:25:51Z

Also, there are compatibility issues that would need to be taken care of...

dbuenzli · 2023-06-23T14:44:51Z

What's the story from a user and driver (say odig which works out of installs) perspective ?

trefis · 2023-06-27T11:25:37Z

What's the story from a user and driver (say odig which works out of installs) perspective ?

I guess that's still open?
In the absence of references to / lookup for assets it would indeed be best if the various drivers used the same convention for picking the parents of their asset, but this PR isn't prescriptive in any way in that regard.

I do hope we can add references to assets in the not too distant future to make this question disappear, in the meantime my intuition is that right now the easiest is probably to use the "package root" as parent of the assets.

Does that somewhat answer your question, or did I totally miss your point?

dbuenzli · 2023-06-27T12:09:20Z

In the absence of references to / lookup for assets it would indeed be best if the various drivers used the same convention for picking the parents of their asset, but this PR isn't prescriptive in any way in that regard.

And yet that's what we need. It was already decided to not prescribe anything for the parent/child relationship and as I suggested at that the time that's not a good idea. We could have had perfectly self-described documents but are moving away from that more and more.

Look at what happend with upstream not prescribing anything for libraries: eco-system mess. Now odoc is perpetuating this.

trefis · 2023-07-04T13:38:54Z

It was already decided to not prescribe anything for the parent/child relationship and as I suggested at that the time that's not a good idea. We could have had perfectly self-described documents but are moving away from that more and more.

I get your point. However, after years of being away from odoc and having not been involved in this decision, I just came back for this "small" PR.
I do not plan to try and argue against decisions which have been made years ago, and certainly not in a PR like this one.

That being said:

I hope this PR will still be beneficial to you, and to everyone else
while odoc itself isn't prescriptive, other tools can and will be; in particular dune (and probably odig) and we are the ones who will write the behavior there, so if you have a precise proposal/request now is still a good time to make it known (or repeat it).

panglesd · 2023-07-04T13:49:47Z

I agree that the decision to make odoc more prescriptive is out of the scope of this PR, which just adds support for assets in a way that conforms to the current state of odoc.

The code looks good, let's merge, thanks @trefis!

dbuenzli · 2023-07-04T19:43:54Z

while odoc itself isn't prescriptive, other tools can and will be; in particular dune (and probably odig) and we are the ones who will write the behavior there, so if you have a precise proposal/request now is still a good time to make it known (or repeat it).

This has been written for ages here and it's mentioned in #59. Interacting with this project remains as frustrating as ever I have to say.

Compile command accepts assets as children

d528a45

trefis requested review from dbuenzli and panglesd June 22, 2023 15:54

trefis commented Jun 22, 2023

View reviewed changes

panglesd approved these changes Jun 23, 2023

View reviewed changes

trefis added 4 commits June 27, 2023 13:19

Generate and targets commands accept --asset args

b604c8a

Test that assets work correctly

6c4c01d

url: s/source/any/

e20e0db

keep processing assets even if some are missing

ff97d11

trefis force-pushed the assets branch from 8ef5773 to ff97d11 Compare June 27, 2023 11:20

trefis added 2 commits June 29, 2023 14:43

Anchor.from_identifier

ade9d4a

old caml is old

9d89354

trefis force-pushed the assets branch from c4572b4 to 4a7afbb Compare July 4, 2023 11:27

tests: sed...

b147ff1

trefis force-pushed the assets branch from 4a7afbb to b147ff1 Compare July 4, 2023 11:57

panglesd merged commit 6f72b2d into ocaml:master Jul 4, 2023
2 of 3 checks passed

trefis deleted the assets branch July 4, 2023 15:18

panglesd mentioned this pull request Sep 20, 2023

Asset references #1002

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rudimentary support for assets #975

Rudimentary support for assets #975

trefis commented Jun 22, 2023

trefis Jun 22, 2023

panglesd Jun 23, 2023

trefis Jun 27, 2023

panglesd Jun 28, 2023 •

edited

trefis Jun 22, 2023

panglesd Jun 23, 2023

panglesd left a comment

panglesd Jun 23, 2023

panglesd Jun 23, 2023

panglesd commented Jun 23, 2023

dbuenzli commented Jun 23, 2023

trefis commented Jun 27, 2023

dbuenzli commented Jun 27, 2023

trefis commented Jul 4, 2023

panglesd commented Jul 4, 2023

dbuenzli commented Jul 4, 2023

Rudimentary support for assets #975

Rudimentary support for assets #975

Conversation

trefis commented Jun 22, 2023

trefis Jun 22, 2023

Choose a reason for hiding this comment

panglesd Jun 23, 2023

Choose a reason for hiding this comment

trefis Jun 27, 2023

Choose a reason for hiding this comment

panglesd Jun 28, 2023 • edited

Choose a reason for hiding this comment

trefis Jun 22, 2023

Choose a reason for hiding this comment

panglesd Jun 23, 2023

Choose a reason for hiding this comment

panglesd left a comment

Choose a reason for hiding this comment

panglesd Jun 23, 2023

Choose a reason for hiding this comment

panglesd Jun 23, 2023

Choose a reason for hiding this comment

panglesd commented Jun 23, 2023

dbuenzli commented Jun 23, 2023

trefis commented Jun 27, 2023

dbuenzli commented Jun 27, 2023

trefis commented Jul 4, 2023

panglesd commented Jul 4, 2023

dbuenzli commented Jul 4, 2023

panglesd Jun 28, 2023 •

edited