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

Add autosectionlabel_full_reference configuration variable #13076

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

andrewjmaguire
Copy link

Subject: Enable the autosectionlabel extension to create more unique references

Feature or Bugfix

  • Feature

Purpose

  • In a large document with many same-named sub-sections, the autosectionlabel extension will not create useful references that can be used to reference these non-uniquely named sub-sections.
    This pull-request includes an option that makes the usable references that include the section name hierarchy in the reference string.

Detail

  • Add autosectionlabel_full_reference configuration variable

Relates

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.

Thanks, that's a nice addition but I'd like you to address the following points (especially for improving test coverage because I think the current algorithm will not work if there are non-section nodes between two sections).

In addition, we'll need a CHANGELOG entry for this one since it's a new feature.

doc/usage/extensions/autosectionlabel.rst Outdated Show resolved Hide resolved
doc/usage/extensions/autosectionlabel.rst Outdated Show resolved Hide resolved
doc/usage/extensions/autosectionlabel.rst Outdated Show resolved Hide resolved
sphinx/ext/autosectionlabel.py Outdated Show resolved Hide resolved
sphinx/ext/autosectionlabel.py Outdated Show resolved Hide resolved
sphinx/ext/autosectionlabel.py Outdated Show resolved Hide resolved
sphinx/ext/autosectionlabel.py Outdated Show resolved Hide resolved
* :ref:`index:Installation:For UNIX users:Linux:Command`
* :ref:`index:Installation:For UNIX users:FreeBSD`
* :ref:`index:Installation:For UNIX users:FreeBSD:Command`
* :ref:`index:Installation:This one's got an apostrophe`
Copy link
Member

Choose a reason for hiding this comment

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

Can you check check with numbers in titles as well as with :math: blocks please? and with literal (double backquotes). Can we also use :ref: in a title? (if not, no need for the coverage).

Additionally, can you test when you specify a custom label before an inner title? as well as content in-between two titles (in other words, try to see if it's possible to construct a doctree with the following tree: "section -> non-section -> section") so that the parent of the second section is a non-section node.

Copy link
Author

@andrewjmaguire andrewjmaguire Oct 28, 2024

Choose a reason for hiding this comment

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

Would adding an image in between suffice? e.g.


Section
-------

Here is an image or I could use a figure?

.. image:: test.png

Sub Section
~~~~~~~~~~~

See previous image...

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, the image and subsection would be sibling nodes and not parent->child nodes. Maybe it's not possible to actually have a parent node that is not a section's node but I need to check that (it's midnight here so I won't do it today).

Copy link
Author

Choose a reason for hiding this comment

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

I have tested with numbers in sections

Copy link
Author

Choose a reason for hiding this comment

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

I have tried to added all the other comments, and added texinfo and latex tests and the test includes another rst file, to check how references worked there.
How is it looking now?

sphinx/ext/autosectionlabel.py Outdated Show resolved Hide resolved
In usage, updated as suggested in review
Added versionadded 8.2 for the new feature

Walked node tree looking only for nodes.section type.
Prefixed internally used functions with _.

Added bool type to add_config_value calls
Used Python f'' string for efficiency

Overrides document.settings.id_prefix so that suitable references
are created in case of any duplicate section hierarchy. Tests for these
are created too.

The autosectionlabel_full_reference tests now include another file
to check multiple file references.

When using autosectionlabel_full_reference in texinfo,
the filename is not prefixed in references. Added texinfo tests.

When using autosectionlabel_full_reference in latex,
the filename is not prefixed in references. Added latex tests

All new tests pass
All type-checks pass
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 have a lot of work to do so I'm only doing a half-review now (sorry). I'll try to come back later this week or the week after, though I will have to travel afterwards (sorry x2).

doc/usage/extensions/autosectionlabel.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@@ -20,7 +20,7 @@
logger = logging.getLogger(__name__)


def get_node_depth(node: Node) -> int:
def _get_node_depth(node: Node) -> int:
Copy link
Member

Choose a reason for hiding this comment

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

Ah maybe I wasn't clear. Old APIs are fine to be kept untouched (because we don't want to break packages) but it's good if new APIs can be designed with a good scope.

sphinx/ext/autosectionlabel.py Outdated Show resolved Hide resolved
Andrew Maguire added 10 commits November 7, 2024 10:33
…ation

Undo _get_node_depth rename
Change _find_node_parents to _get_all_node_parent_section_titles to describe
more fully what the function does and _ prefix is just for internal use.
This reverts commit 9072d62.
This makes the full section id available to be used for cross references.

Note: The original short id is still needed by other functionality.
I.e. the full id cannot replace the short one, `node['ids'][0] = labelid`
because then TOC and sectnum:: functionality is broken in the HTML output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants