Add ability to call generate commands on asset units #1185
Conversation
|
(I'm not very proud of the name of the CLI option: |
panglesd
force-pushed
the
generate-assets
branch
from
2de1814
to
5741d4b
Compare
src/odoc/bin/main.ml
Outdated
| let source = source_of_args source_root source_file in | ||
| let file = Fs.File.of_string input_file in | ||
| Rendering.generate_odoc ~renderer:R.renderer ~warnings_options ~syntax | ||
| ~output:output_dir ~extra_suffix ~source ~sidebar extra file | ||
| ~output:output_dir ~extra_suffix ~source ~sidebar ~asset_path extra file |
There was a problem hiding this comment.
I'm not a fan of adding more arguments to this command. This argument makes no sense in most cases and needs to be checked. Similarly, Document.Asset seems a bit out of place, as it interacts with nothing else, it could be out of the document thing entirely.
What about adding a different command for generating assets ? html-generate-asset, latex-generate-asset, etc..
There was a problem hiding this comment.
The same is true for --source and --source-root, but OK, I'll do that!
There was a problem hiding this comment.
(the Document.Asset will be inconvenient to remove in my opinion, unless we remove the old way of attaching assets, which would be a good thing I think)
|
I have added support in the driver for the asset pipeline, following the convention. This means, we now have images working for packages following the odig convention!
|
| @@ -135,6 +135,10 @@ let pkg_to_dir_map () = | |||
| | "doc" :: _pkg :: "odoc-pages" :: _ -> | |||
| Logs.debug (fun m -> m "Found odoc page: %a" Fpath.pp fpath); | |||
|
|
|||
| (Fpath.Set.add Fpath.(v prefix // fpath) odoc_pages, others) | |||
| | "doc" :: _pkg :: "odoc-assets" :: _ -> | |||
There was a problem hiding this comment.
Unrelated to this PR but pages and assets are taken from the list of installed files for a package. I find that unnecessary and potentially buggy. A package could install doc in an other package or this file could be outdated (dune install), which might crash the driver.
I think the doc/pkgname folder should be scanned independently of pkg_contents. I think the same for libraries but there might be cases that require this?
|
There's a broken asset in |
This removes a deprecated-before-use pipeline, and allows to finally remove the extra_documents in renderers
|
This has been rebased on top of #1188. |
| @@ -24,7 +24,6 @@ type input = | |||
| type 'a t = { | |||
| name : string; | |||
| render : 'a -> Types.Block.t option -> Types.Document.t -> page list; | |||
| extra_documents : 'a -> input -> Types.Document.t list; | |||
src/odoc/rendering.ml
Outdated
| let doc = Renderer.document_of_asset asset_file unit in | ||
| render_document renderer ~output ~sidebar:None ~extra_suffix ~extra doc; |
There was a problem hiding this comment.
There was a lot of simplification in this PR, why not continue and remove assets from the document and the renderer ?
document_of_asset seems very orthogonal to the rest of the document (it's just the Asset constructor which shares nothing with the other document types) and because of render_document and the renderer type, the content of the asset file has to go through a Format buffer, which seems totally unnecessary.
I think all of this could be removed.
There was a problem hiding this comment.
True! (I started that and decided to stop, thinking you would not see it, so I stashed everything. Time to stash pop!)
Co-authored-by: Jules Aguillon <jules@j3s.fr>
I'm being pragmatic and fixing this in this PR. |
The pipeline for asset was missing the generate command. This PR simply adds that!
The driver will need to be updated to follow the convention for installed asset described in #1113 (section: "Assets for installed packages").