Display description text in docs groups

[lud]

Related to #2104

Still a work in progress. If we go in this direction we should add docs as well.

There are some limits with this solution:

• Since we want the description from default_group_for_docs, it means that each doc node can return a different description for a given group. I made the choice to ignore those descriptions based on the group title.
• As we want to keep the group order defined in moduledoc, the retriever will fetch those, possibly containing descriptions. I made the choice to not replace descriptions from moduledoc by descriptions returned by the config callback.
• Node groups will temporarily contain a title/description map, only to be replaced with just the title once the groups are built in the modulenode. This avoids a lot of "when is_binary() ... when is_map()" stuff and avoids to refactor many tests. This also allows to have each group rendered only once. A drawback is that in the module_summary function we are reconciliating the groups once again where each doc node could already bear the full group. Not a big deal imho.
• Since those groups are rendered, they must behave like most nodes. But they are not nodes for now. I guess we could add a struct for those, but it's not the real problem. What is more important is that I had to hardcode "text/markdown" and wrap the text in %{"en" => text }, as module and doc nodes are extracted from documentation. I could have written a specialized doc_ast render function though.
• I added some classes on the html elements which may not be needed. Same for the IDs. This was mostly to test the rendering.

Besides that, it works.

[josevalim]

The TODO above is precisely about moving this logic to the retriever. Given we are already changing the group structure, maybe now is a good time to go ahead and do it? Basically, by adding a group column to the groups upfront?

[lud]

Yeah I saw this TODO and in my first attempt I wanted to provide docs_groups with all the docs inside.

But to avoid having duplicate data, to me it means that we basically remove the :docs key from the ModuleNode struct. And that means refactoring a lot of tests to look for expected data in a list of groups instead of just matching on the content of the docs key. Lot more code to change and review. But I can do it if you think it's alright.

[lud]

ah sorry I missed the part about a group column. Should be simple enough.

[lud]

Alright, please see that last commit: aed460e

(There were other commits before since you last replied).

I'm not sure this is what you expect. The retriever is computing the groups, so the renderer part is only fetching that, which is what we wanted.

But,

• There is duplicated data. The ModuleNode still has its :docs key with the DocNode list because many tests look into that for assertions. We can update those tests to look into the groups instead, with a lot of Enum.find calls :) and then delete the ModuleNode :docs key.
• Those DocNodes in the :docs key have nil :rendered_doc because we only render the copy of the doc nodes that are inside group nodes.

Or maybe it is something else you had in mind?

[josevalim]

The code looks great, thank you ! 🤩

There is duplicated data. The ModuleNode still has its :docs key with the DocNode list because many tests look into that for assertions. We can update those tests to look into the groups instead, with a lot of Enum.find calls :) and then delete the ModuleNode :docs key.

I think it is worth giving this a try! Perhaps we could have a helper function in tests, called find_doc(...) or find_group(...) to help make life easier. Could you please try that as well? I understand it is a lot of work though, but this is some of the refactoring we have been wanting to do for quite some time (basically do less work on the formatters and more on the retrievers).

[lud]

Hey @josevalim , have you had time to review my comments?

[josevalim]

Sorry for the delay, I am on holidays so I have limited time to stay on top of my inbox. I am reviewing this now but probably merge it only when I am back. :)

[lud]

Hello @josevalim no problem, it was just to be sure this was not forgotten. Take your time. We probably need to add some docs before mergeing.

@garazdawi if you have time can you tell me if it works as you would expect?

Thank you both.