-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add tests for :manpage:
inside a title
#11705
Add tests for :manpage:
inside a title
#11705
Conversation
Could you open an issue for the title rendering piece? A |
sphinx/builders/html/__init__.py
Outdated
@@ -425,7 +425,7 @@ def render_partial(self, node: Node | None) -> dict[str, str]: | |||
if node is None: | |||
return {'fragment': ''} | |||
|
|||
doc = new_document('<partial node>') | |||
doc = new_partial_document() |
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.
Perhaps just setting an attribute?
doc = new_partial_document() | |
doc = new_document('<partial node>') | |
doc['is_partial'] = True |
and then doc.get('is_partial', False)
to test.
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.
I wanted to keep the old behaviour in case there was someone somewhere who relied on that. Also, it's to remind us that we have possible partial documents somewhere and that we always need to be aware of.
On the other hand, using an attribute may lead false positive where a user would define this specific attribute. When using the source + special name we may avoid these cases (is_partial may be a "simple" attribute name that may be used in some projects).
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.
I'm averse to introducing new infrastructure around this if it isn't something we want to keep long-term. So I would prefer the attribute approach (keeping the document name the same), we could use _is_partial
or some mangled name if we need to.
A
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.
we could use _is_partial or some mangled name if we need to.
That's fine by me then! Actually I don't know whether we'll keep it long-term or not since I don't know if we will be able to fix this issue on partial documents.
I'll do the update next week (I'me quite busy these days).
:manpage:
inside a title:manpage:
inside a title
So, color me surprised but #11825 actually fixed this PR. So I'm only adding some tests now to ensure that the behaviour is as expected. |
Fix #11673.
By fixing this, I observed a weird behaviour. In order to determine the HTML title of the page, we store a real title node and not just a string. Then, we create a fake document and render it. However, by doing so, the Sphinx transformations are never called on that fake document, but the writer is called! That means that there are some visitor methods that assume that a transformation was called, but this is not the case for those partial documents.
I think we need to rethink about this in order to decouple the interaction. I don't know if there are other methods that could fail because of that.