-
Notifications
You must be signed in to change notification settings - Fork 340
Display description text in docs groups #2113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -169,7 +169,14 @@ defmodule ExDoc.Formatter.HTML.Templates do | |||
|
|||
def module_summary(module_node) do | |||
# TODO: Maybe it should be moved to retriever and it already returned grouped metadata | |||
ExDoc.GroupMatcher.group_by(module_node.docs_groups, module_node.docs, & &1.group) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry I missed the part about a group column. Should be simple enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theDocNode
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 theModuleNode
:docs
key. - Those
DocNode
s in the:docs
key havenil
: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?
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:
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.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.%{"en" => text }
, as module and doc nodes are extracted from documentation. I could have written a specializeddoc_ast
render function though.Besides that, it works.