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

[rst] Add level option to rubric directive #12506

Merged
merged 6 commits into from
Jul 14, 2024

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Jul 3, 2024

This PR adds a level option to the rubric directive, which propagates a level attribute to the rubric node,
and allows renderers to select a specific heading level.

Logic for this attribute has been added to the HTML5 and LaTeX builder:

.. rubric:: This is a rubric

.. rubric:: A rubric with a heading level
   :level: 2

is converted to HTML:

<p class="rubric">This is a rubric</p>
<h2 class="rubric">A rubric with a heading level</h2>

and is converted to LaTeX:

\subsubsection*{This is a rubric}
\subsection*{A rubric with a heading level}

currently, other writers are not changed (and thus will ignore the level attribute)


A rubric is "an informal heading that doesn't correspond to the document's structure" (https://docutils.sourceforge.io/docs/ref/rst/directives.html#rubric)

This can be used in situations where a section heading is not allowed, since it would break the document structure, for example in admonitions (https://gist.github.com/chrisjsewell/0c5827add50074fef0937e2543e955b4).

This is disallowed: and emits a docutils ERROR:

.. note::

    Sub-section heading
    -------------------

    Sub-sub-section heading
    .......................

but could now be replaced in a way you would more or less get the same output, but without issues:

.. note::

    .. rubric:: Sub-section heading
	   :level: 2

    .. rubric:: Sub-sub-section heading
	   :level: 3

It is of note, that in MyST-Parser this is already built-in to the parser;
if a heading is encountered when match_titles=False, rather than emit an ERROR, it creates a rubric with a level equal to the number of #: https://github.com/executablebooks/MyST-Parser/blob/d448abf395c29bb649f81fba5c1a2bc49e195cc0/myst_parser/mdit_to_docutils/base.py#L869

and currently overrides the HTML rubric visitor: https://github.com/executablebooks/MyST-Parser/blob/d448abf395c29bb649f81fba5c1a2bc49e195cc0/myst_parser/sphinx_ext/main.py#L43-L47

@chrisjsewell
Copy link
Member Author

@AA-Turner this is somewhat a solution to issues we discussed in #12492 (comment) and e.g. #10532, where you want to allow headings within nested content

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I think it is a very useful feature because there are soooo many times when I want to use === headings in my docstrings but cannot because the parser is not happy (telling me that whatever title I use is not recognized I think?) so I generally fall back to using rubrics.

However, the good thing about headers is that it creates labels, but I don't think it's the case with rubrics by default. So it's not entirely a fix of having --- in "notes".

Comment on lines +522 to +524
if level := node.get("level"):
self.body.append(f'</h{level}>\n')
else:
Copy link
Member

Choose a reason for hiding this comment

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

Add an assert on the allowed level (to avoid people injecting invalid attributes). And make sure that it's also of type int.

The level should not be at most 6 since otherwise it's not a valid HTML tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

a warning for unsupported level values has been added

Copy link
Member

Choose a reason for hiding this comment

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

Say node['level'] = 9, the warning would be emitted on visit_ with no opening tag, but the closing </h9> tag would still be added. This isn't possible using the directive but we should guard against it (and test appropriately).

3: 'subsubsection',
4: 'paragraph',
5: 'subparagraph',
}.get(node.get('level', 0), 'subsubsection')
Copy link
Member

Choose a reason for hiding this comment

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

Add also an assert here because people are not meant to inject a level deeper than what should be allowed. And possibly add a warning to indicate that a level outside 0-5 (and not 0-6!) is not allowed.

Actually, if someone uses level 6, then it's fine in HTML but in LaTeX you would have a subsubsection which may bigger than a subpargraph. So for level 6, it should 'subparagraph' (I'm not sure if the document class has a subparagraph, maybe you should check all levels + non-levels in tests)

Copy link
Member Author

@chrisjsewell chrisjsewell Jul 14, 2024

Choose a reason for hiding this comment

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

This now uses the "correct" logic, using the self.sectionnames and self.top_sectionlevel variables.

a warning for unsupported level values has also been added

@@ -5,3 +5,7 @@ test-markup-rubric

.. rubric:: This is
a multiline rubric

.. rubric:: A rubric with a heading level
:level: 2
Copy link
Member

Choose a reason for hiding this comment

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

Check levels 1 to 6 and invalid levels as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@chrisjsewell
Copy link
Member Author

However, the good thing about headers is that it creates labels, but I don't think it's the case with rubrics by default.

can you explain a bit more?
can't you just do:

.. rubric:: title
  :name: my-label

@picnixz
Copy link
Member

picnixz commented Jul 12, 2024

Ah, I mean that when you have a title like

Hello
-----

On HTML builds, I think you automatically have an anchor (or is it only with the autosectionlabel extension?) but not with rubrics (but maybe I'm wrong here). By the way, I didn't know about the :name: option... good to know now...

@chrisjsewell chrisjsewell merged commit 41b363d into sphinx-doc:master Jul 14, 2024
21 checks passed
@chrisjsewell chrisjsewell deleted the rubric-level branch July 14, 2024 03:53
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. I may need to consider reverting this for the 7.4 release, there are a few points that need resolving.

On a theoretical basis, I am not fully convinced that adding this option to rubric plays with the doctree. This seems to be side-stepping semantics to achieve presentational end-goals (e.g. the level of a heading). We generally don't do that -- note e.g. in reST you can't directly jump to a particular "heading level", as opposed to Markdown where the number of # denotes the level.

I'm open to being convinced, though -- but I would want to be in a position where we could advocate for this feature going into upstream Docutils, given it has doctree-semantics implications for a core directive.

A

Comment on lines +31 to +32
* Add ``level`` option to :rst:dir:`rubric` directive.
Patch by Chris Sewell.
Copy link
Member

Choose a reason for hiding this comment

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

This should go at the end (chronological ordering)

Comment on lines +379 to +380
.. rst:directive:option:: level: n
:type: number from 1 to 6
Copy link
Member

Choose a reason for hiding this comment

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

I think level is a little vague, perhaps heading-level or section-level?

Comment on lines +522 to +524
if level := node.get("level"):
self.body.append(f'</h{level}>\n')
else:
Copy link
Member

Choose a reason for hiding this comment

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

Say node['level'] = 9, the warning would be emitted on visit_ with no opening tag, but the closing </h9> tag would still be added. This isn't possible using the directive but we should guard against it (and test appropriately).

Comment on lines +36 to +38
.. rubric:: A rubric with a heading level 7
:level: 7
:class: myclass
Copy link
Member

Choose a reason for hiding this comment

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

This only tests that the directive is rejecting numbers outwith 1-6, we need to test the writers independently.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jul 14, 2024

This seems to be side-stepping semantics to achieve presentational end-goals

Well... I would suggest this is kind of what you were trying to do in #10532

As we've discussed recently, in the docutils doctree, in is never ever allowed to but a section within a non-structural node, so you will never achieve what you were trying to do there, i.e. placing sections in adminitions

In HTML there are two distinct concepts/tags section (which wrap other content and nest) and title (which are standalone), the problem with reStructuredText is that it does not provide any way to distinguish between the two: a title always has to also be a section

In most other document syntaxes, not just Markdown, they allow for non-structural headings in some form:

Given that Sphinx in a multi-input format framework, it should accommodate for that in some form.

@AA-Turner AA-Turner added this to the 7.4.0 milestone Jul 15, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants