Asset references #1002

panglesd · 2023-09-20T18:58:47Z

Assets were introduced in #975.

This PR adds references to assets.

Resolving of reference to asset work like this:

Search the asset in the current page's asset (or the explicitely given page)
if not found, recurse in the parent page.

In case of unqualified reference (e.g. "caml.gif", page-name."caml.gif") the asset possibility is tested last.

References to assets is a prerequesite to:

Search Support for search in odoc #972
Medias Syntax for including images #985

trefis

LGTM modulo nitpicks

trefis · 2023-09-21T09:33:30Z

src/xref2/errors.ml

@@ -291,7 +294,8 @@ type what =
  | `Module_type_u_expr of Component.ModuleType.U.expr
  | `Child_module of string
  | `Child_page of string
-  | `Reference of Reference.t ]
+  | `Reference of Reference.t
+  | `Asset_reference of Reference.Asset.t ]


Was that strictly necessary?

Of course, not strictly necessary.
In #972 it was used to make the message a little bit more precise, since we know the reference resolved passed in the command line is an asset reference.
I can remove, I don't have an opinion on this.

test/xref2/references_to_assets.t/run.t

dbuenzli · 2023-09-21T13:28:32Z

Do you actually have a design doc somewhere on how all this is supposed to work because I'm afraid to say it doesn't make much sense to me. In an opam install how do you know which assets is attached to which page ?

panglesd · 2023-09-25T13:47:34Z

No, I don't have a design doc, but I finally realized that all these parent-child relationship do not make sense in terms of opam installs, and that we will need to do something about that before going further...
Thanks for the insight!

dbuenzli · 2023-09-25T14:28:27Z

If I understand correctly these parent-child relationships are mainly there to handle the package versioning on ocaml.org (or at least that's how it was sold to me when I said it wouldn't be such a good idea not to have self-describing documents).

If we follow the odig conventions (which as far as I know dune does). Then maybe to generate the docs of a package we can simply:

Make index.mld the root of a parent-child relationship for the package (note odig generates an index.mld if there is none provided).
Any module found in the package is made a child of index.mld
Any page found in odoc-pages (cf convention) is made a child of it.
Any asset found (using file extensions) in odoc-pages and/or odoc-assets is attached to index.mld.

Since IIUC this PR a child can access it's parent assets then any asset is available for use in all the doc strings and manuals of the package.

For users it's a simple model: your package simply has a folder with odoc pages and assets which are all accessible flately. This should be entirely sufficient for 98% of the packages out there.

Does that make sense ?

panglesd · 2023-09-26T10:26:53Z

Yes, this makes sense to me.

I think simply adding some documentation about this on odoc's doc, maybe on the one about the driver, would suffice.
(And, making sure dune keeps following the convention when it adds supports for assets.)

Maybe, one day, one would want to use this parent-child relationship even inside a package, but I'm convinced that the package-flat hierarchy is enough for 99% of the usecases.

panglesd · 2023-09-26T10:31:00Z

(which reminds me: this PR is lacking documentation update on assets)

panglesd · 2023-11-03T17:42:50Z

The state of this PR is:
There are some refactoring suggested by @jonludlam: Populate the environment for once at the beginning, to have all assets available, instead of looking them up recursively on demand.

Also, I think a decision regarding the referencing and resolving of pages should be made before making a hard decision on this one!

Reference can now be toward assets. When resolving them, look up the assets of the current page, and if not found, recurse in the parent page. Signed-off-by: Paul-Elliot <[email protected]>

Signed-off-by: Paul-Elliot <[email protected]>

panglesd · 2024-01-08T08:40:30Z

doc/parent_child_spec.mld

@@ -175,17 +175,17 @@ installed and might be used by a different driver.
 In order for drivers to build consistent documentation for a package, the
 following convention should be followed.

- [.mld] pages are installed in a package's [share] directory, under the
+- [.mld] pages and assets are installed in a package's [share] directory, under the


Should assets be installed in a particular directory, or with a particular name?
With the proposed convention, assets with extension .mld are recognized as pages. But I think this is fine. If really needed, we could find some ways to work around the restriction with extension .mld_... ?

This seems fine to me, as long as we mention this caveat.

panglesd · 2024-01-08T08:50:34Z

I implemented @jonludlam's suggestion to populate the environment with available assets, which I agree makes the code better.

I also added an explanation on the convention for assets in installed packages.

Currently, the resolving of assets is going "up" in the parent-child hierarchy, looking for the asset. This might not be the "final" strategy: we might want to change this to be closer to what is done with pages (see the discussion on that in #1011). However, I believe that it is safe to start with this strategy, as I don't expect a change in the strategy to break (future) existing documentation.

So in my opinion, this PR is ready to be merged.

gpetiot

A few typos, and a few suggestions, otherwise looks good to me!

doc/ocamldoc_differences.mld

doc/odoc_for_authors.mld

doc/parent_child_spec.mld

gpetiot · 2024-01-29T15:32:48Z

doc/parent_child_spec.mld

@@ -175,17 +175,17 @@ installed and might be used by a different driver.
 In order for drivers to build consistent documentation for a package, the
 following convention should be followed.

- [.mld] pages are installed in a package's [share] directory, under the
+- [.mld] pages and assets are installed in a package's [share] directory, under the


This seems fine to me, as long as we mention this caveat.

src/model/reference.ml

src/model_desc/paths_desc.ml

src/xref2/env.ml

Julow

Nice :)

doc/parent_child_spec.mld

Julow · 2024-02-01T09:36:10Z

src/model/reference.ml

+        | _ -> expected [ "page" ] location |> Error.raise_exception)
+    | next_token :: tokens -> (
+        match kind with
+        | `TUnknown -> `Dot (label_parent next_token tokens, identifier)


I don't think label_parent makes sense here. Perhaps it should be page ?

Julow · 2024-02-01T09:37:19Z

src/model/reference.ml

+            not_allowed ~what:"Page label"
+              ~in_what:"the last component of a reference path" ~suggestion
+              location
+            |> Error.raise_exception)


I don't understand this error message and suggestion. I think expected [ "page" ] would be a reasonable error.

Thanks, there was something wrong here!

Pages reference cannot have a parent, and I want to preserve this behavior in this PR. Should be better now.

src/model/reference.ml

Julow · 2024-02-01T10:03:50Z

src/xref2/ref_tools.ml

@@ -784,7 +824,12 @@ let resolve_reference_dot_sg env ~parent_path ~parent_ref ~parent_sg name =
      Error (`Find_by_name (`Any, name))

 let resolve_reference_dot_page env page name =
-  L.in_page env page name >>= resolved1
+  match (L.in_page env page name, page) with


That's not the usual shape of functions in this module. To make this more efficient and maintainable, it should match on the result of a single search like resolve_reference_dot_type below.
The correct find function is not in the Find module yet, it is currently inlined in L.in_page and should be extended for assets.

src/xref2/ref_tools.ml

Julow · 2024-02-01T10:10:25Z

test/xref2/references_to_assets.t/run.t

@@ -0,0 +1,71 @@
+In this file, we test the resolving of asset references.


Nothing in this test shows a working asset reference. Shouldn't caml.gif_hidden be renamed ?
Also, the only reference that seems to be intended to work do not test page parents (there's no other_page."caml.gif").

Yes, at the end there are working asset references. The assets are not passed in the html generation phase, which creates "dead links" just as there are dead links when you do not html-generate some of the pages), but the test checks that the links point to the right place.
Do you think it would be better to actually pass some image in the html generation, to compare the path more accurately?

I went ahead and passed assets in the test.

Fixed convention's wrong installed folder for pages and assets Fixed error message when a page reference has a parent Fixed typo in desc Add precision in test Signed-off-by: Paul-Elliot <[email protected]>

Signed-off-by: Paul-Elliot <[email protected]>

Co-authored-by: Guillaume Petiot <[email protected]>

Signed-off-by: Paul-Elliot <[email protected]>

panglesd · 2024-03-18T10:36:28Z

Closing now since asset referencing will change subsequently soon. But some of the code will be used!

trefis approved these changes Sep 21, 2023

View reviewed changes

panglesd mentioned this pull request Sep 26, 2023

Medias in odoc #1005

Closed

Julow mentioned this pull request Sep 28, 2023

Doc: parent-child convention for installed packages #1011

Merged

panglesd force-pushed the asset-references branch from f479436 to cbbbc84 Compare September 28, 2023 11:22

panglesd assigned jonludlam and Julow Sep 29, 2023

panglesd force-pushed the asset-references branch from 70f83f2 to c10fb42 Compare September 29, 2023 13:27

EmileTrotignon added the after-2.4 label Dec 5, 2023

panglesd force-pushed the asset-references branch from c10fb42 to 7c00147 Compare December 22, 2023 13:29

panglesd added 9 commits January 8, 2024 09:32

Introduce resolving of asset referencing

7df2c1f

Reference can now be toward assets. When resolving them, look up the assets of the current page, and if not found, recurse in the parent page. Signed-off-by: Paul-Elliot <[email protected]>

fix wrong kind in warning for missing label

92a9862

Signed-off-by: Paul-Elliot <[email protected]>

ocamlformat

78577ae

Signed-off-by: Paul-Elliot <[email protected]>

ref to assets: fix test explanation

68f572d

Signed-off-by: Paul-Elliot <[email protected]>

Assets: add documentation

c87f9eb

Signed-off-by: Paul-Elliot <[email protected]>

asset reference test: compatiblity

d046ffe

Signed-off-by: Paul-Elliot <[email protected]>

Add changelog entry

790dc28

Signed-off-by: Paul-Elliot <[email protected]>

Populate environment with assets

d9911e3

Signed-off-by: Paul-Elliot <[email protected]>

Asset referencing: compatibility

367601a

Signed-off-by: Paul-Elliot <[email protected]>

panglesd force-pushed the asset-references branch from 14f51a4 to 367601a Compare January 8, 2024 08:32

Assets: Update convention for installed packages

272d8e7

Signed-off-by: Paul-Elliot <[email protected]>

panglesd commented Jan 8, 2024

View reviewed changes

gpetiot approved these changes Jan 29, 2024

View reviewed changes

Julow reviewed Feb 1, 2024

View reviewed changes

panglesd and others added 4 commits February 6, 2024 09:38

Assets: review comments

41ef4d7

Fixed convention's wrong installed folder for pages and assets Fixed error message when a page reference has a parent Fixed typo in desc Add precision in test Signed-off-by: Paul-Elliot <[email protected]>

Remove unused ` Resolved `` variant for page reference

19ed6f2

Signed-off-by: Paul-Elliot <[email protected]>

Apply suggestions from code review

266858b

Co-authored-by: Guillaume Petiot <[email protected]>

Assets: actually pass assets in test

81a2df4

Signed-off-by: Paul-Elliot <[email protected]>

panglesd closed this Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset references #1002

Asset references #1002

panglesd commented Sep 20, 2023

trefis left a comment

trefis Sep 21, 2023

panglesd Sep 21, 2023

dbuenzli commented Sep 21, 2023

panglesd commented Sep 25, 2023

dbuenzli commented Sep 25, 2023

panglesd commented Sep 26, 2023 •

edited

Loading

panglesd commented Sep 26, 2023

panglesd commented Nov 3, 2023

panglesd Jan 8, 2024

gpetiot Jan 29, 2024

panglesd commented Jan 8, 2024

gpetiot left a comment

gpetiot Jan 29, 2024

Julow left a comment

Julow Feb 1, 2024

Julow Feb 1, 2024

panglesd Feb 6, 2024

Julow Feb 1, 2024

Julow Feb 1, 2024

panglesd Feb 6, 2024

panglesd Feb 6, 2024

panglesd commented Mar 18, 2024

		@@ -0,0 +1,71 @@
		In this file, we test the resolving of asset references.

Asset references #1002

Asset references #1002

Conversation

panglesd commented Sep 20, 2023

trefis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbuenzli commented Sep 21, 2023

panglesd commented Sep 25, 2023

dbuenzli commented Sep 25, 2023

panglesd commented Sep 26, 2023 • edited Loading

panglesd commented Sep 26, 2023

panglesd commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panglesd commented Jan 8, 2024

gpetiot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panglesd commented Mar 18, 2024

panglesd commented Sep 26, 2023 •

edited

Loading