Add sphinx.util.nodes:make_id() to generate better node_id

3 files changed, 44 insertions, 10 deletions

diff --git a/sphinx/domains/std.py b/sphinx/domains/std.py
index 2d2b47499..4961b2cf6 100644
--- a/sphinx/domains/std.py
+++ b/sphinx/domains/std.py
@@ -30,7 +30,7 @@ from sphinx.locale import _, __
 from sphinx.roles import XRefRole
 from sphinx.util import ws_re, logging, docname_join
 from sphinx.util.docutils import SphinxDirective
-from sphinx.util.nodes import clean_astext, make_refnode
+from sphinx.util.nodes import clean_astext, make_id, make_refnode
 from sphinx.util.typing import RoleFunction
 
 if False:
@@ -256,14 +256,8 @@ def make_glossary_term(env: "BuildEnvironment", textnodes: Iterable[Node], index
         # node_id is given from outside (mainly i18n module), use it forcedly
         pass
     elif document:
-        node_id = nodes.make_id('term-' + termtext)
-        if node_id == 'term':
-            # "term" is not good for node_id.  Generate it by sequence number instead.
-            node_id = 'term-%d' % env.new_serialno('glossary')
-
-        while node_id in document.ids:
-            node_id = 'term-%d' % env.new_serialno('glossary')
-
+        node_id = make_id(env, document, 'term', termtext)
+        term['ids'].append(node_id)
         document.note_explicit_target(term)
     else:
         warnings.warn('make_glossary_term() expects document is passed as an argument.',
diff --git a/sphinx/util/nodes.py b/sphinx/util/nodes.py
index f33a6a001..7c7300c60 100644
--- a/sphinx/util/nodes.py
+++ b/sphinx/util/nodes.py
@@ -28,6 +28,7 @@ if False:
     # For type annotation
     from typing import Type  # for python3.5.1
     from sphinx.builders import Builder
+    from sphinx.environment import BuildEnvironment
     from sphinx.utils.tags import Tags
 
 logger = logging.getLogger(__name__)
@@ -435,6 +436,28 @@ def inline_all_toctrees(builder: "Builder", docnameset: Set[str], docname: str,
     return tree
 
 
+def make_id(env: "BuildEnvironment", document: nodes.document,
+            prefix: str = '', term: str = None) -> str:
+    """Generate an appropriate node_id for given *prefix* and *term*."""
+    node_id = None
+    if prefix:
+        idformat = prefix + "-%s"
+    else:
+        idformat = document.settings.id_prefix + "%s"
+
+    # try to generate node_id by *term*
+    if prefix and term:
+        node_id = nodes.make_id(idformat % term)
+        if node_id == prefix:
+            # *term* is not good to generate a node_id.
+            node_id = None
+
+    while node_id is None or node_id in document.ids:
+        node_id = idformat % env.new_serialno(prefix)
+
+    return node_id
+
+
 def make_refnode(builder: "Builder", fromdocname: str, todocname: str, targetid: str,
                  child: Node, title: str = None) -> nodes.reference:
     """Shortcut to create a reference node."""
diff --git a/tests/test_util_nodes.py b/tests/test_util_nodes.py
index b1a6b7d1d..76ba51800 100644
--- a/tests/test_util_nodes.py
+++ b/tests/test_util_nodes.py
@@ -17,7 +17,9 @@ from docutils.parsers import rst
 from docutils.utils import new_document
 
 from sphinx.transforms import ApplySourceWorkaround
-from sphinx.util.nodes import NodeMatcher, extract_messages, clean_astext, split_explicit_title
+from sphinx.util.nodes import (
+    NodeMatcher, extract_messages, clean_astext, make_id, split_explicit_title
+)
 
 
 def _transform(doctree):
@@ -27,6 +29,7 @@ def _transform(doctree):
 def create_new_document():
     settings = frontend.OptionParser(
         components=(rst.Parser,)).get_default_values()
+    settings.id_prefix = 'id'
     document = new_document('dummy.txt', settings)
     return document
 
@@ -180,6 +183,20 @@ def test_clean_astext():
     assert 'hello world' == clean_astext(node)
 
 
+def test_make_id(app):
+    document = create_new_document()
+    assert make_id(app.env, document) == 'id0'
+    assert make_id(app.env, document, 'term') == 'term-0'
+    assert make_id(app.env, document, 'term', 'Sphinx') == 'term-sphinx'
+
+    # when same ID is already registered
+    document.ids['term-sphinx'] = True
+    assert make_id(app.env, document, 'term', 'Sphinx') == 'term-1'
+
+    document.ids['term-2'] = True
+    assert make_id(app.env, document, 'term') == 'term-3'
+
+
 @pytest.mark.parametrize(
     'title, expected',
     [