Skip to content
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

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

Merged
merged 2 commits into from Sep 18, 2023

Conversation

percevalw
Copy link
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
Member

pawamoy commented Sep 17, 2023

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

Copy link
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!

src/mkdocstrings/extension.py Outdated Show resolved Hide resolved
src/mkdocstrings/extension.py Outdated Show resolved Hide resolved
src/mkdocstrings/extension.py Outdated Show resolved Hide resolved
tests/test_extension.py Outdated Show resolved Hide resolved
tests/test_extension.py Outdated Show resolved Hide resolved
tests/test_extension.py Outdated Show resolved Hide resolved
Co-authored-by: Timothée Mazzucotelli <pawamoy@pm.me>
Copy link
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
16 of 17 checks passed
elif carry_text:
el.tail = (el.tail or "") + carry_text
carry_text = ""
elif self._remove_duplicated_headings(el):
Copy link
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.

elif carry_text:
el.tail = (el.tail or "") + carry_text
carry_text = ""
elif self._remove_duplicated_headings(el):
found = True
break
Copy link
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
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
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
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.

None yet

3 participants