Skip to content

Remove duplicated headings for docstrings nested in tabs/admonitions#610

Merged
pawamoy merged 2 commits intomkdocstrings:mainfrom
percevalw:fix-duplicated-headings
Sep 18, 2023
Merged

Remove duplicated headings for docstrings nested in tabs/admonitions#610
pawamoy merged 2 commits intomkdocstrings:mainfrom
percevalw:fix-duplicated-headings

Conversation

@percevalw
Copy link
Copy Markdown
Contributor

@percevalw percevalw commented Sep 17, 2023

Hi,

As mentioned in #609, nesting a docstring in another block (e.g., tab or admonition) produces duplicated headings.
At the moment, the extension postprocessor only checks inside the immediate root children when removing duplicated heading.

Some markdown extensions move the produced docstring in a nested html node : we only need to search deeply when removing duplicatas.

@percevalw percevalw changed the title Remove duplicated headings for docstrings nested in tabs/admonions Remove duplicated headings for docstrings nested in tabs/admonitions Sep 17, 2023
@pawamoy
Copy link
Copy Markdown
Member

pawamoy commented Sep 17, 2023

Amazing! I can confirm that both the issue exists and the fix works!

Copy link
Copy Markdown
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I had noticed the issue but didn't get time to report it. You reported it, and fixed it, that's awesome 😍

Just a few nitpicks!

Comment thread src/mkdocstrings/extension.py Outdated
Comment thread src/mkdocstrings/extension.py Outdated
Comment thread src/mkdocstrings/extension.py Outdated
Comment thread tests/test_extension.py Outdated
Comment thread tests/test_extension.py Outdated
Comment thread tests/test_extension.py Outdated
Co-authored-by: Timothée Mazzucotelli <pawamoy@pm.me>
@percevalw percevalw force-pushed the fix-duplicated-headings branch from d06656e to d0f06d4 Compare September 17, 2023 22:27
Copy link
Copy Markdown
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK perfect, thanks so much again!

@pawamoy pawamoy merged commit e2123a9 into mkdocstrings:main Sep 18, 2023
elif carry_text:
el.tail = (el.tail or "") + carry_text
carry_text = ""
elif self._remove_duplicated_headings(el):
Copy link
Copy Markdown
Member

@oprypin oprypin Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a wrong behavior: "If text was carried, don't bother stepping into the current element". There is no connection between the two, so it shouldn't be an else.

carry_text = ""
elif self._remove_duplicated_headings(el):
found = True
break
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a wrong behavior: "If duplicated headings were found in a child element, stop scanning the rest of the document upwards". Why?

Copy link
Copy Markdown
Member

@pawamoy pawamoy Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's no specific "why" and the assumption is that duplicated headings appear in a single child element. Do you recommend that we try and remove them from the whole subtree?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this was my assumption, that transformations would only nest the duplicated heading block without repeating it. Breaking early should improve the performance if the tree is large, but I have not idea by how much or if this was necessary.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Results from the child element are being propagated to the parent loop and it gets interrupted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants