art-w / sherlodoc

Fuzzy type search for OCaml documentation
MIT License
73 stars 6 forks source link

Awesome search bar and question about enabling it with multiple packages #29

Open mbarbin opened 8 months ago

mbarbin commented 8 months ago

Hello,

I spotted the new sherlodoc's search bar in dune's 3.14 changelog and couldn't wait to try it out. I've now enabled it in a project.

That project has multiple opam packages. Right now, each package gets its own search bar. Have you considered adding a global search bar on the index page that covers all packages? I haven't given it much thought, and merging the indexes might lead to problems, so perhaps this would need consideration (I don't know, just a thought).

Thank you for this feature and its smooth integration with dune. The setup was a breeze! I appreciate all your efforts!

EmileTrotignon commented 8 months ago

You already have this if you build your doc with dune build @doc-new. In that case, the search bar will also include dependencies, which might not be ideal.

My reasoning when writing the dune rules was that @doc is primarily meant for uploading your docs to something like github.io, that is for users of the project. This means that the doc of separate packages could be uploaded separately, and also that the size of the js search database is a concern. On the contrary, @doc-new is meant for the dev of the project, because it builds the docs of deps and private libs, so a single search bar is more appropriate, and database size is not a concern because it is meant for local use.

EmileTrotignon commented 8 months ago

However, I think it might be beneficial for this behavior to configurable in dune. This needs to be done in dune, not sherlodoc.

I am not sure of what the exact look of such config would be.

mbarbin commented 8 months ago

Hi @EmileTrotignon

Thank you for your response :smiley:

doc-new

Thanks for letting me know about it, I had missed it. Earlier today I attempted to build a doc with @doc-new but encountered some issues. Here's what I did:

  1. I had a browser open, pointing to my local file index in _build/default/_doc/_html/index.html.
  2. I ran dune build @doc-new in the terminal. This generated a lot of output messages, as I am used to.
  3. I refreshed the browser page and tried to use the search bar, but didn't notice any changes.

Later, I noticed a new directory _build/default/_doc_new/ and assumed this is where the command outputs its files. However, I couldn't find an HTML index there.

$ ls _build/default/_doc_new/
html  index  odoc

$ find _build/default/_doc_new -path '*.html'
(no hit)

Could this be due to the following in my root dune file?

(env
 (dev
  (odoc
   (warnings fatal))))

Could pulling dependencies cause this command to fail because more files are processed? If so, would it be reasonable to request that odoc ignore warnings when building dependencies, similar to how dune ignores warnings in vendor libs?

Search bar in home page

Aside from the multi-package question, would there be any downside to including the search bar on the odoc's home page?

For example, compare: https://mbarbin.github.io/error-log/ (doesn't have the search bar) with: https://mbarbin.github.io/error-log/error-log/index.html (does have it)

Multi package

Once I can successfully use doc-new, I'll be able to provide more useful feedback. For now, I'm not entirely clear on the differences between @doc and @doc-new. From your description, it seems like I might need something that's a hybrid of the two.

For instance, consider the project I originally linked, https://mbarbin.github.io/eio-rpc/ :

  1. It is uploaded to github.io
  2. The multiple packages are part of a whole unit, so a unified search would make more sense
  3. I'm not sure if including dependencies is necessary

What happens when you select a search hit in a dependency btw? Does it redirect somewhere?

Dune configuration

Regarding the configuration, I'm typically used to put odoc options in the dune file at the root of a project. Maybe a new field there would make sense? This part of the config can be made profile dependent, so you could, for example, enable doc-new only in the dev profile:

(env
 (dev
  (odoc
   (mode new))))

I realize my feedback is starting to sound like a Christmas wish list - my apologies! As a casual and quite happy user, I'm more than willing to try out more things if you find my feedback valuable. Thanks again for all your hard work!

art-w commented 8 months ago

Yes I agree! Libraries that are developed and documented together should be cross-searchable, since they are intended to be used in conjunction :) The odoc bland listing of packages on the homepage is inconvenient to navigate as it provides little guidance, a search bar would be very welcome here (cc @panglesd and @Julow as I don't know how feasible this is on odoc side?)

My understanding is that the @doc-new mode is intended for local developper usage (akin to an odig switch-wide documentation but for a specific project). In your case, it would also generate and index the documentation of Eio, the Stdlib, etc, so the search results would be less focused on documenting your library for your users (but links to external dependencies would remain internal to your documentation pages and not redirect elsewhere). I believe I managed to run it so you can test it on your cool libraries :) (but as you can see, it searches everywhere and the database size ends up crazy big... the next release of dune should improve the search quality by favoring your libs over dependencies but I haven't tested it yet)

The generation of @doc-new html pages does seem to fail because of your odoc strict config in your dune file, so your suggestion to behave differently for dependencies warning makes sense! (but again I'm not sure how feasible this is?)

(Btw, long time no see @mbarbin, I hope you are doing great! Would love to catch up over a drink if you ever come by Paris :) )

EmileTrotignon commented 8 months ago

I refreshed the browser page and tried to use the search bar, but didn't notice any changes.

That is the main reason that there is a new rule, the new rule have a different structure, which meant that if they just replaced the old rules it would break a lot of things. They also have the difference of best use case we mentionned.

Could pulling dependencies cause this command to fail because more files are processed? If so, would it be reasonable to request that odoc ignore warnings when building dependencies, similar to how dune ignores warnings in vendor libs?

Your may be right, but I am not sure this is way your doc-new folder is empty. If your diagnosis is right this could likely be done in dune with minimal effort, but @jonludlam would probably be a better person to ask that question to.

Could you try removing the (warnings fatal) from your config ?

Aside from the multi-package question, would there be any downside to including the search bar on the odoc's home page?

The reason its not here is that this is not an odoc page, but a page generated by dune : https://github.com/ocaml/dune/blob/581f852c5144b03be0067a93289e90262fa8cbc6/src/dune_rules/odoc.ml#L512

We could add a search bar to it, but it would means duplicating (and re-doing) some work from odoc. This is unfortunate because I agree it would be nice ergonomics.

Maybe a good way to do that would be to remove this html page and replace it by a real odoc page (an mld generated by dune)

it seems like I might need something that's a hybrid of the two.

I think you likely need @doc. @doc-new would include the documentation of all your dependencies which is too much for your needs. If you have private libs in your project they would also be documented when they are not meant for the end users. However, I think you and Arthur are right and I need to change the scope of the search bar for @doc.

Regarding the configuration, I'm typically used to put odoc options in the dune file at the root of a project.

Putting it in dune-project sounds right. It would not be choosing between doc-new and doc, but a sherlodoc-specific settings to choose the scope of the search bar.

Maybe the default would be a large scope with the option to have multiple smaller search bar for very large projects.

Julow commented 8 months ago

The odoc bland listing of packages on the homepage is inconvenient to navigate as it provides little guidance, a search bar would be very welcome here

This page is generated by the driver and it should be feasible dune-side. (or is generated by Odoc but there's no reason to keep it that way) Odoc do not have a notion of packages, which could mean something different depending on the driver. For example, ocaml.org has a search bar for packages and has a page that lists all the versions of a package.

jonludlam commented 8 months ago

Could pulling dependencies cause this command to fail because more files are processed? If so, would it be reasonable to request that odoc ignore warnings when building dependencies, similar to how dune ignores warnings in vendor libs?

Your may be right, but I am not sure this is way your doc-new folder is empty. If your diagnosis is right this could likely be done in dune with minimal effort, but @jonludlam would probably be a better person to ask that question to.

This should already be implemented, modulo bugs - we're supposed to pass the 'quiet' flag to external libs (things that come from opam rather than in your tree), and this should suppress the --warn-error flag: https://github.com/ocaml/dune/blob/main/src/dune_rules/odoc.ml#L237-L246 - if that's not working please file an issue!

Aside from the multi-package question, would there be any downside to including the search bar on the odoc's home page?

The reason its not here is that this is not an odoc page, but a page generated by dune : https://github.com/ocaml/dune/blob/581f852c5144b03be0067a93289e90262fa8cbc6/src/dune_rules/odoc.ml#L512

Note that in doc-new odoc generates all the pages, so this isn't an issue.

art-w commented 8 months ago

Yeah I can confirm that no html files are generated by @doc-new with (odoc (warnings fatal)) (only the odoc.support/ and sherlodoc.js are produced) and that it complains about ERROR: Warnings have been generated from dependencies. Otherwise it works correctly when the warnings are not fatal... Should the issue be opened in dune or odoc?

Would it be possible for @doc to use the @doc-new homepage, with its search bar and packages listing, just without the "Switch-installed packages" section?

mbarbin commented 8 months ago

Just to bring up another (unrelated?) issue that's been adding to my confusion: there are warnings that still pop up and don't cause dune build @doc to fail, even when the fatal option is set. I'm guessing there's a variety of warnings, and these are the ones that are just noisy and generally ignored?

...
Warning: Failed to resolve reference ...

I vaguely remember reading about this issue somewhere, but can't recall where exactly. For now, I've been ignoring them (without trying to filter them out) in the hope that they'll be removed eventually.

we're supposed to pass the 'quiet' flag to external libs

Judging by the output of dune build @doc-new on dependencies (verbose), it doesn't seem like this is working as expected at the moment.

mbarbin commented 8 months ago

Thank you so much for looking into my build issue, @art-w, and for publishing a working build! That's really kind of you. The @doc-new mode seems like a very useful feature for local development indeed. I'll give it a try over the next few days and will return to this thread if I have any feedback. I was able to rebuild locally by editing the config as suggested.

(Btw, long time no see @mbarbin, I hope you are doing great! Would love to catch up over a drink if you ever come by Paris :) )

Thanks! You know, keeping caml and currying on. Absolutely! I'll make sure to reach out on my next visit to Paris :)

jonludlam commented 8 months ago

Would it be possible for @doc to use the @doc-new homepage, with its search bar and packages listing, just without the "Switch-installed packages" section?

This is rather annoyingly hard to do - and one of the reasons the layout of doc-new had to change. My goal is to ensure the doc-new target works for the local docs case first, but then to add (somehow) the capability to cut down the output so it covers the use case of 'doc' (ie, principally publishing the docs on your own site). Eventually I'd like to see the 'doc' target deprecated.

jonludlam commented 8 months ago

Yeah I can confirm that no html files are generated by @doc-new with (odoc (warnings fatal)) (only the odoc.support/ and sherlodoc.js are produced) and that it complains about ERROR: Warnings have been generated from dependencies. Otherwise it works correctly when the warnings are not fatal... Should the issue be opened in dune or odoc?

Drat! The issue is in dune - open it there and assign it to me please!

mbarbin commented 8 months ago

I vaguely remember reading about this issue somewhere, but can't recall where exactly. For now, I've been ignoring them (without trying to filter them out) in the hope that they'll be removed eventually.

I found this again, I was referring to this discussion between @jonludlam and @Julow https://github.com/ocaml/odoc/pull/784#discussion_r756772278

I'll add a +1 to the linked issue there. Thanks!