Allow more hierarchy to be compatible with `-L` and `-P`. #1233

panglesd · 2024-11-04T08:16:33Z

In the initial implementation of -L and -P we added too many constraints: the trees below the roots (both -L and -P) had to all be disjoint.

This prevented some hierarchy, such as the one where pages are put in pkgname/ and libraries in pkgname/libname/: indeed, -L pkgname/libname would not be disjoint from -P pkgname/.

This PR relax the constraint, -L roots have to be disjoints, and -P roots have to be disjoint, but there is no constraint between a -P and a -L root.

It also relaxes the constraint the when linking modules, the module has to be below a -L.

Tests are updated to use the simpler hierarchy.

I'll update the driver in another PR!

gpetiot · 2024-11-04T12:40:19Z

src/odoc/bin/main.ml

+     else Ok ())
+    >>= fun () ->
+    (if not (Antichain.check (lib_roots |> List.map ~f:snd)) then
+       Error (`Msg "Paths given to all -L options must be disjoint")


Not really relevant to this PR, but wouldn't it be cleaner if Antichain.check would return (unit, some_error) result instead of a boolean? The error message can be customized with map_error or as a parameter to the check function.

Yes, that was not very readable, hopefully I've improved it in the last commit.

jonludlam · 2024-11-04T22:24:06Z

test/integration/link_opts.t/run.t


-Specified current package is wrong:
+  $ odoc link -P pkg:h/pkg -L otherlib:h/otherpkg h/pkg/libname/test.odoc


nit: do you want -L otherlib:h/otherpkg/otherlib here instead?

Yes. I've cleaned up the test a bit (some options were not really necessary for the test and made it less readable). Hopefully it is better now!

jonludlam · 2024-11-05T10:34:33Z

src/odoc/bin/main.ml

+    | [] -> None
+    | _ ->
+        find_map
+          ~f:(fun (pkg, path) ->


These names are slightly misleading as pkg might actually be a lib. I know that was also the case before, but we might as well fix that now.

panglesd · 2024-11-05T10:49:22Z

I have made more tests with the new (doc-lib)less hierarchy, and there is a small issue.
When we have -P pkgname/ -L pkgname/libname, we don't want the content of pkgname/libname to be added to the accessible pages for pkgname (at least, not in the sidebar: the libraries are already included in the sidebar).
(The problem is shown in this commit)

An image is worth a thousand words

The sidebar for odoc, suffering from this:

I have solved this by excluding -L subdirectories from -P roots (a5f767c). Another option would have to add a metadata tag @sidebar omit which tells odoc to omit this subhierarchy from the sidebar. I think this second option is better, otherwise the name clashes are more likely.

jonludlam · 2024-11-05T11:18:45Z

That @sidebar omit is a little odd as it's only omitting it as a subdirectory of another 'sidebar root'. It doesn't seem odd to me that things that are configured as 'sidebar roots' are never permitted to be subdirs of another 'sidebar root' so I think your current fix isn't unreasonable at all.

`-P` is now allowed to contain `-L` options, and modules do not need to be below a `-L`. This gives less contraints to the hierarchy decided by the driver. In particular, this allows the hierarchy without `doc/` and `lib/` folders: pages are put directly in `pkgname/` and modules in `pkgname/libname/`. The original hierarchy is still possible.

By canonical I mean the one without doc and lib intermediate folders.

When a -L is a subfolder of -P (eg -P pkgname/ -L pkgname/libname), we don't want the content of the -L root to be included in the -P root.

gpetiot · 2024-11-07T12:18:48Z

src/odoc/bin/main.ml

@@ -601,81 +601,49 @@ end = struct
          match f x with Some _ as result -> result | None -> find_map ~f l)


By the way find_map is in Odoc_utils.List

gpetiot · 2024-11-07T12:42:09Z

I have made more tests with the new (doc-lib)less hierarchy, and there is a small issue. When we have -P pkgname/ -L pkgname/libname, we don't want the content of pkgname/libname to be added to the accessible pages for pkgname (at least, not in the sidebar: the libraries are already included in the sidebar). (The problem is shown in this commit)

Is there a previous discussion about this? I'm interested in having more context before forming an opinion, because naively it looks to me that it's something that should be handled in the driver (meaning that redundancies in the arguments would be intentional and should be preserved in the sidebar). But I don't have all the context :)

panglesd · 2024-11-08T08:08:56Z

I think some context can be understood by reading the discussion starting from this comment.

We can also do a quick call if you'd like.

jonludlam · 2024-11-08T10:15:06Z

The alternate suggestions for how to create the perfect sidebar can happen in a later PR - this one is pretty foundational for some other work so let's get this in to unblock the other PRs.

Several changes: - Entries are now defined in the `odoc_index` library, - Entries can have new kinds (pages, source, ...) - Indexes have the form of "skeletons of entries", that can be folded. - Indexes can be created by odoc with the `odoc compile-index` command, and then consumed by sherlodoc. These changes come from: - ocaml/odoc#1228 - ocaml/odoc#1232 - ocaml/odoc#1233 - ocaml/odoc#1244 - ocaml/odoc#1250 - ocaml/odoc#1251

panglesd added the no changelog This pull request does not need a changelog entry label Nov 4, 2024

panglesd force-pushed the allow-more-hierarchies branch from 40727d1 to f5f0687 Compare November 4, 2024 10:35

gpetiot reviewed Nov 4, 2024

View reviewed changes

panglesd force-pushed the allow-more-hierarchies branch 2 times, most recently from e3b10bf to b32814b Compare November 4, 2024 18:17

jonludlam reviewed Nov 4, 2024

View reviewed changes

panglesd force-pushed the allow-more-hierarchies branch from 5431307 to 6f36972 Compare November 5, 2024 08:58

jonludlam reviewed Nov 5, 2024

View reviewed changes

panglesd added 10 commits November 6, 2024 09:26

Hierarchy: update test to use the "canonical hierarchy"

5d937aa

By canonical I mean the one without doc and lib intermediate folders.

Test: Move sidebar to a more logical directory

4a0f843

Test: canonical hierarchy

836c324

Test: hierarchy with separate doc and lib folders

2ff3cc1

Small refactor of Antichain.check

6d7db98

Test: improve link_opts test readability

79c3ca6

Test: Add more sidebar test

9bf48e1

Exclude -L subfolders from -P roots

861a2c0

When a -L is a subfolder of -P (eg -P pkgname/ -L pkgname/libname), we don't want the content of the -L root to be included in the -P root.

Fix misleading name

7eedbc8

panglesd force-pushed the allow-more-hierarchies branch from 746a58f to 7eedbc8 Compare November 6, 2024 08:27

panglesd mentioned this pull request Nov 6, 2024

Driver: New hierarchy #1236

Merged

gpetiot reviewed Nov 7, 2024

View reviewed changes

Use Odoc_utils for List.find_map compat.

ebb4689

jonludlam merged commit 6bb7627 into ocaml:master Nov 8, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow more hierarchy to be compatible with `-L` and `-P`. #1233

Allow more hierarchy to be compatible with `-L` and `-P`. #1233

panglesd commented Nov 4, 2024

gpetiot Nov 4, 2024

panglesd Nov 4, 2024

jonludlam Nov 4, 2024

panglesd Nov 5, 2024 •

edited

Loading

jonludlam Nov 5, 2024

panglesd commented Nov 5, 2024

jonludlam commented Nov 5, 2024

gpetiot Nov 7, 2024

gpetiot commented Nov 7, 2024

panglesd commented Nov 8, 2024

jonludlam commented Nov 8, 2024


		Specified current package is wrong:
		$ odoc link -P pkg:h/pkg -L otherlib:h/otherpkg h/pkg/libname/test.odoc

		@@ -601,81 +601,49 @@ end = struct
		match f x with Some _ as result -> result \| None -> find_map ~f l)

Allow more hierarchy to be compatible with -L and -P. #1233

Allow more hierarchy to be compatible with -L and -P. #1233

Conversation

panglesd commented Nov 4, 2024

gpetiot Nov 4, 2024

Choose a reason for hiding this comment

panglesd Nov 4, 2024

Choose a reason for hiding this comment

jonludlam Nov 4, 2024

Choose a reason for hiding this comment

panglesd Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

jonludlam Nov 5, 2024

Choose a reason for hiding this comment

panglesd commented Nov 5, 2024

jonludlam commented Nov 5, 2024

gpetiot Nov 7, 2024

Choose a reason for hiding this comment

gpetiot commented Nov 7, 2024

panglesd commented Nov 8, 2024

jonludlam commented Nov 8, 2024

Allow more hierarchy to be compatible with `-L` and `-P`. #1233

Allow more hierarchy to be compatible with `-L` and `-P`. #1233

panglesd Nov 5, 2024 •

edited

Loading