From 1719275c9ff688b87e28448851f780da0ae2b9a3 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 18:49:11 -0400 Subject: [PATCH] fix(changelog-md): cite a line the entry has, and accept an anchored :rfc: role (#588, #592) * ResidualMarkupError named the source entry file but numbered its lines against the converted body, so the number it printed could not exist in the file it pointed at. Rule 6 joins each block onto one line: measured on the 1.5.0 entry the body is 55 lines against the file's 580, and four roles written on source lines 467 and 468 were all reported as "line 46". The bound alone settles it -- a number that cannot exceed 55 cannot name a line in a 580-line file. convert_traced() now returns a source map beside the Markdown, recording which line of the entry each stretch of output came from, and residual() resolves every hit through it. That also separates the hits: constructs joined onto one output line become one complaint each, at their own lines, in the entry's order rather than the pattern list's. residual() still takes a bare string, and then labels its numbers "converted line N" instead of passing an unusable number off as a location. * Rule 2 accepted only :rfc:`NNNN`, so :rfc:`NNNN#section-3` -- the spelling Sphinx also accepts, and the one anyone citing a section reaches for -- fell past the rule that exists for it and was rejected as a role the rules do not cover. Both spellings convert now, with the anchor carried into the target. The link text comes from sphinx.roles._format_rfc_target rather than being invented here, so an entry reads the same in CHANGELOG.md as in the rendered history; an anchor of a shape that helper does not name still reaches the guard rather than being carried into a link nobody checked. * Two supporting changes the map needs. Masking a code span is now length- preserving, so a match offset in the masked text is an offset in the Markdown; no pattern counts markers, so nothing about what matches moves. And rule 6 collapses runs of blank lines structurally rather than with a \n{3,} substitution over the joined text, which would have shifted every offset -- same result between blocks, and one better before the first block, where the substitution used to leave an empty line behind. * The `` literal branch of the guard reports one complaint per literal rather than per output line, and had no test of its own until now. Regeneration is unchanged: 36 of the 37 committed entries convert byte for byte as before, and the 37th differs by exactly the one bullet added here. python util/changelog_md.py --check exits 0. tests/project/ green: 64 passed, 469 subtests. The 13 new cases in tests/project/test_changelog_md.py were run against the committed converter first and fail there (46 tests, 9 failures and 2 errors, exit 1) and pass with the fix (47 tests, exit 0); util/changelog_md.py rises from 96% to 98% statement and branch coverage, the remainder being the __main__ guard and a pre-existing break in read_toctree(). --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 17 ++ tests/project/test_changelog_md.py | 254 ++++++++++++++++++++++++++++ util/changelog_md.py | 263 ++++++++++++++++++++++++----- 4 files changed, 493 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49cc9d432..c45439e26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ The largest release since 1.0, and the first recorded here as it happened rather This is the resolution of #548, which reported `TransType.L2TP` (115) as registered nowhere and proposed binding `L2TPv2` there. That binding is wrong rather than merely awkward: [RFC 3931](https://datatracker.ietf.org/doc/html/rfc3931) ยง4.1.1 gives 115 to *L2TPv3 over IP*, whose session header is "free of any restrictions imposed by coexistence with L2TPv2 and L2F" and carries **no version nibble at all**, so a v2 parser cannot even detect that the datagram is not its own. Measured, it produced `version=4`, `tunnelid=0x5678` and `sessionid=0xff03` from the top half of a Session ID and two octets of the PPP frame behind it. 115 is a missing *class*, not a missing registration, and stays unbound until an `L2TPv3` class exists; no dissector was invented here to fill it. The reasoning is now recorded in `pcapkit.protocols.link.l2tp` rather than only in a test, and `register_protocol_code`'s worked example -- which named `L2TPv2` at 115 -- names `L2TPv3` instead (#548). - **Added** -- `tests/protocols/test_dispatch_reachability_unit.py`, the coverage #548 asked for: every `ProtocolBase` descendant whose `__index__` returns an enum member is checked to be reachable under that code in the registry its enum *type* designates, read from the same `_CODE_DESTINATIONS` table backing `code=` so the two cannot drift. Where `test_dispatch_registry_unit.py` walks the 38 entries that exist and checks each parses, this walks the classes and catches one nothing registered at all -- the shape in which `OSPF` once shipped reachable from no table. 23 claims verified, no gaps; a companion case injects a gap and confirms the audit reports it, so the guard cannot rot into a permanently green no-op (#548). +- **Fixed** -- both halves of what the `Changelog drift` gate told an author, in `util/changelog_md.py`. `ResidualMarkupError` named the entry file but numbered its lines against the *converted* body, which rule 6 joins onto one line per block: measured on this entry, the body is 55 lines against the file's 580, so a reported number could not reach most of the file at all, and four roles written on two source lines were all reported as `line 46`. The conversion now carries a source map -- which line of the entry each stretch of output came from -- so every complaint cites a line of the file the message names, one complaint per construct rather than one per joined line, in the entry's own order (#588). Rule 2 separately accepted only the bare-number spelling of Sphinx's `:rfc:` role, so a citation of [RFC 6554 Section 3](https://datatracker.ietf.org/doc/html/rfc6554#section-3) fell past the rule that exists for it and was reported as a role the rules do not cover; both spellings now convert, with the anchor carried into the link target and the link text taken from Sphinx's own so that the Markdown and the rendered history say the same thing about the same page (#592). Regenerating `CHANGELOG.md` is byte-identical over all 37 committed entries, so the fix changes what the gate *says* and nothing about what it emits. Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 93c724f49..b02128dea 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -567,6 +567,23 @@ pull requests between #326 and #509. -- the shape in which ``OSPF`` once shipped reachable from no table. 23 claims verified, no gaps; a companion case injects a gap and confirms the audit reports it, so the guard cannot rot into a permanently green no-op (#548). +* **Fixed** -- both halves of what the ``Changelog drift`` gate told an author, + in ``util/changelog_md.py``. ``ResidualMarkupError`` named the entry file but + numbered its lines against the *converted* body, which rule 6 joins onto one + line per block: measured on this entry, the body is 55 lines against the file's + 580, so a reported number could not reach most of the file at all, and four + roles written on two source lines were all reported as ``line 46``. The + conversion now carries a source map -- which line of the entry each stretch of + output came from -- so every complaint cites a line of the file the message + names, one complaint per construct rather than one per joined line, in the + entry's own order (#588). Rule 2 separately accepted only the bare-number + spelling of Sphinx's ``:rfc:`` role, so a citation of :rfc:`6554#section-3` + fell past the rule that exists for it and was reported as a role the rules do + not cover; both spellings now convert, with the anchor carried into the link + target and the link text taken from Sphinx's own so that the Markdown and the + rendered history say the same thing about the same page (#592). Regenerating + ``CHANGELOG.md`` is byte-identical over all 37 committed entries, so the fix + changes what the gate *says* and nothing about what it emits. Preceded by ``1.5.0a1`` (2026-09-15), ``1.5.0b1`` and ``1.5.0b2`` (both 2026-09-18) and ``1.5.0b3`` (2026-09-19), all published as prereleases and so diff --git a/tests/project/test_changelog_md.py b/tests/project/test_changelog_md.py index 66e793935..67d3c8f3c 100644 --- a/tests/project/test_changelog_md.py +++ b/tests/project/test_changelog_md.py @@ -18,6 +18,12 @@ version strings), and ``--check``, which is what a CI gate calls and is worthless if it cannot fail. +One class covers the guard's *message* rather than its verdict, because the guard +is what blocks a merge and the message is all the author gets. Its assertions are +deliberately about the entry file rather than about the wording: a cited line is +read back out of the entry and has to hold the construct it was cited for, which +is the one property a line number in a diagnostic exists to have. + Almost everything here builds its own two-file changelog tree in a temporary directory rather than reading the repository's. That keeps the rule tests honest -- each entry is written to exercise one rule, instead of hoping the real @@ -92,6 +98,27 @@ def _load_generator(): """ +def padded_entry(*tail: str, padding: int = 60) -> str: + """An entry whose single bullet wraps over *padding* source lines, then *tail*. + + Rule 6 joins the lot onto one output line, so the converted body is a handful + of lines while the entry file is dozens of them. That gap is the whole of + #588: a line number counted in the body cannot reach most of the file, so a + construct written in *tail* is at a line the body does not have. + + """ + lines = [ + '9.9.9 -- 2026-01-02', + '===================', + '', + '* **Added** -- a bullet whose prose wraps over many source lines, so that', + ] + lines += [f' padding line {number} of the wrapped bullet.' + for number in range(1, padding + 1)] + lines += [f' {line}' for line in tail] + return '\n'.join(lines) + '\n' + + class ChangelogTreeMixin: """Builds a throwaway changelog tree for one test.""" @@ -131,6 +158,52 @@ def test_rule_2_rfc_role_becomes_a_datatracker_link(self) -> None: self.assertIn('[RFC 4303](https://datatracker.ietf.org/doc/html/rfc4303)', markdown) self.assertNotIn(':rfc:', markdown) + def test_rule_2_rfc_role_with_a_section_anchor_becomes_a_deep_link(self) -> None: + # #592: ``(\d+)`` between the backticks accepted digits and nothing else, + # so the anchored spelling Sphinx also accepts fell past rule 2 and was + # rejected by the guard -- which reported the *role* as uncovered when only + # the spelling ever was. The fragment has to reach the target, or the link + # lands at the top of a 100-page RFC. + markdown = self.convert(ENTRY.replace(':rfc:`4303`', ':rfc:`4303#section-2.1`')) + + self.assertIn( + '[RFC 4303 Section 2.1](https://datatracker.ietf.org/doc/html/rfc4303#section-2.1)', + markdown, + ) + self.assertNotIn(':rfc:', markdown) + + def test_rule_2_anchor_titles_follow_sphinx(self) -> None: + # The link text is Sphinx's, not one invented here, so an entry reads the + # same in CHANGELOG.md as in the rendered history. Measured against + # ``sphinx.roles._format_rfc_target``: it titles three anchor prefixes and + # leaves every other anchor as written. + base = 'https://datatracker.ietf.org/doc/html/rfc' + cases = { + '6554#section-3': f'[RFC 6554 Section 3]({base}6554#section-3)', + '8200#section-4.5': f'[RFC 8200 Section 4.5]({base}8200#section-4.5)', + '6275#appendix-B': f'[RFC 6275 Appendix B]({base}6275#appendix-B)', + '793#page-5': f'[RFC 793 Page 5]({base}793#page-5)', + '9293#introduction': f'[RFC 9293#introduction]({base}9293#introduction)', + '9293#section': f'[RFC 9293 Section]({base}9293#section)', + '4303': f'[RFC 4303]({base}4303)', + } + + for target, expected in cases.items(): + self.assertEqual(self.convert(f':rfc:`{target}`\n').strip(), expected, + f'rule 2 mis-rendered :rfc:`{target}`') + + def test_rule_2_leaves_an_anchor_it_cannot_read_to_the_guard(self) -> None: + # The widened pattern is deliberately not "anything between the backticks". + # An anchor it cannot read has to reach the guard and be reported, rather + # than being carried into a link whose text nobody checked. + markdown = self.convert(ENTRY + '\nSee :rfc:`6554#section 3` for more.\n') + + self.assertIn(':rfc:', markdown) + self.assertTrue( + any('role' in problem for problem in changelog_md.residual(markdown)), + 'an unreadable anchor was neither converted nor reported', + ) + def test_rule_3_double_backtick_literal_becomes_a_code_span(self) -> None: markdown = self.convert() @@ -173,6 +246,22 @@ def test_rule_6_paragraphs_and_bullets_are_unwrapped(self) -> None: f'{line[:50]!r} looks like a wrapped continuation line', ) + def test_rule_6_collapses_blank_runs_and_drops_a_leading_one(self) -> None: + # Carrying the source map through rule 6 means the blank rows are counted + # structurally rather than collapsed afterwards by a ``\n{3,}`` substitution + # over the joined text. Same answer between blocks -- a run of blank lines + # is one paragraph break however long it is -- and a better one before the + # first block, where the substitution used to leave two empty lines. + markdown = self.convert( + '\n\n9.9.9 -- 2026-01-02\n===================\n\n\n\n' + 'One paragraph.\n\n\nAnother.\n' + ) + + self.assertEqual( + markdown, + '## 9.9.9 -- 2026-01-02\n\nOne paragraph.\n\nAnother.\n', + ) + class NewestEntryTests(ChangelogTreeMixin, unittest.TestCase): """How the generator decides which entry is the current release.""" @@ -344,6 +433,14 @@ def test_markup_quoted_inside_a_code_span_is_not_a_leftover(self) -> None: self.assertEqual(changelog_md.residual(markdown), []) + def test_an_rfc_role_with_an_anchor_is_not_a_leftover(self) -> None: + # The gate-level half of #592: rule 2 covers the ``:rfc:`` role, so an entry + # citing a section of an RFC must pass rather than be refused as markup the + # rules do not cover. #590's entry was rewritten to work around this. + markdown = self.convert(ENTRY.replace(':rfc:`4303`', ':rfc:`4303#section-2.1`')) + + self.assertEqual(changelog_md.residual(markdown), []) + def test_the_real_entries_are_all_within_the_subset(self) -> None: directory = changelog_md.INDEX.parent / 'changelog' if not directory.is_dir(): @@ -358,6 +455,163 @@ def test_the_real_entries_are_all_within_the_subset(self) -> None: ) +class ComplaintLocationTests(ChangelogTreeMixin, unittest.TestCase): + """Where the guard says a leftover construct is -- #588. + + The guard blocks merges, so its message is the whole of the author's + experience of it. These do not check the wording; they check that a cited line + read back out of the entry holds the construct it was cited for, which the + numbers counted against the converted body could not do. + + No ``subTest`` here on purpose: this pytest has no ``pytest-subtests``, so a + failing subtest leaves its parent reported as passed. + + """ + + def complaints(self, entry: str) -> list[str]: + """Render *entry*, which must be refused, and return the cited complaints.""" + index = self.make_tree(entry=entry) + + with self.assertRaises(changelog_md.ResidualMarkupError) as error: + changelog_md.render(index) + + return [line.strip() for line in str(error.exception).split('\n') + if line.strip().startswith('line ')] + + def cited(self, complaints: list[str]) -> list[int]: + """The line numbers *complaints* point at.""" + numbers = [] + for complaint in complaints: + found = re.match(r'line (\d+):', complaint) + self.assertIsNotNone(found, f'{complaint!r} cites no line') + assert found is not None + numbers.append(int(found.group(1))) + return numbers + + def test_a_cited_line_holds_the_construct_it_was_cited_for(self) -> None: + entry = padded_entry('and then :data:`sys.modules` at the very end.') + + number, = self.cited(self.complaints(entry)) + + self.assertIn(':data:`sys.modules`', entry.split('\n')[number - 1], + f'line {number} of the entry does not hold the cited construct') + + def test_a_cited_line_can_lie_past_the_end_of_the_converted_body(self) -> None: + # The bound that made the old numbers provably wrong: they were counted in + # the body, so they could never reach a construct written below it. + entry = padded_entry('and then :data:`sys.modules` at the very end.') + body = changelog_md.convert(entry) + + number, = self.cited(self.complaints(entry)) + + self.assertGreater( + number, len(body.split('\n')), + 'the cited line is inside the converted body, so it is a body line ' + 'number rather than a line of the entry the message names', + ) + self.assertLessEqual(number, len(entry.split('\n'))) + + def test_constructs_joined_onto_one_line_keep_their_own_lines(self) -> None: + entry = padded_entry( + 'first :data:`sys.modules`,', + 'then :func:`importlib.import_module`,', + 'and last :class:`dict`.', + ) + joined = [line for line in changelog_md.convert(entry).split('\n') + if ':data:' in line] + self.assertEqual(len(joined), 1, 'the three roles were meant to be joined') + self.assertIn(':class:', joined[0], 'the three roles were meant to be joined') + + numbers = self.cited(self.complaints(entry)) + + self.assertEqual(len(set(numbers)), 3, + f'three constructs on three source lines cited {numbers}') + for number, role in zip(numbers, (':data:', ':func:', ':class:')): + self.assertIn(role, entry.split('\n')[number - 1], + f'line {number} does not hold {role}') + + def test_complaints_arrive_in_the_entrys_order(self) -> None: + # Separate paragraphs, so the two are on different lines in either frame. + # What changes is the order: the patterns are listed with the hyperlink + # before the substitution, and a reader walks the file, not the pattern list. + entry = ENTRY + ( + '\nA |substitution| reference.\n' + '\nA `link `_ reference.\n' + ) + + numbers = self.cited(self.complaints(entry)) + + self.assertEqual(len(numbers), 2, 'expected one substitution and one link') + self.assertEqual(numbers, sorted(numbers), 'complaints are out of order') + self.assertIn('|substitution|', entry.split('\n')[numbers[0] - 1]) + self.assertIn('`link `_', entry.split('\n')[numbers[1] - 1]) + + def test_an_unreached_double_backtick_literal_is_located_too(self) -> None: + # Rule 3's ``[^`]+`` cannot cross a backtick, so a literal holding one is + # copied through and only the guard catches it. That branch of the guard had + # no test of its own. + entry = padded_entry('an ``a`b`` literal rule 3 cannot reach.') + + complaints = self.complaints(entry) + number, = self.cited(complaints) + + self.assertIn('`` literal', complaints[0]) + self.assertIn('``a`b``', entry.split('\n')[number - 1]) + + def test_two_unreached_literals_are_two_located_complaints(self) -> None: + # One complaint per literal rather than per output line: rule 6 joins these + # two source lines into one, and "one of these is wrong" is not a location. + # + # Two lines rather than one because rule 3 pairs the nearest backticks it + # can: given two unconvertible literals side by side on a single line, it + # bridges the closing pair of the first to the opening pair of the second. + # It runs before rule 6 joins, so separate source lines are out of its reach. + entry = padded_entry('an ``a`b`` literal,', 'and a ``c`d`` literal.') + + complaints = self.complaints(entry) + numbers = self.cited(complaints) + + self.assertEqual(len(complaints), 2, complaints) + self.assertEqual(len(set(numbers)), 2, numbers) + self.assertIn('``a`b``', entry.split('\n')[numbers[0] - 1]) + self.assertIn('``c`d``', entry.split('\n')[numbers[1] - 1]) + + def test_residual_without_a_source_map_says_which_frame_it_counted(self) -> None: + # ``residual`` is callable on a bare string, and then there is nothing to map + # through. It has to admit that rather than pass a body line number off as a + # line of an entry -- which is precisely what #588 was. + markdown = changelog_md.convert(ENTRY + '\nSee :mod:`pcapkit.const` for more.\n') + + unmapped = changelog_md.residual(markdown) + mapped = changelog_md.residual( + *changelog_md.convert_traced(ENTRY + '\nSee :mod:`pcapkit.const` for more.\n')) + + self.assertEqual(len(unmapped), 1) + self.assertTrue(unmapped[0].startswith('converted line '), unmapped[0]) + self.assertEqual(len(mapped), 1) + self.assertTrue(mapped[0].startswith('line '), mapped[0]) + + def test_the_real_entries_map_every_offset_to_a_line_they_have(self) -> None: + directory = changelog_md.INDEX.parent / 'changelog' + if not directory.is_dir(): + self.skipTest(f'{directory} is absent (docs/ is pruned from a source tarball)') + + for entry in sorted(directory.glob('*.rst')): + text = entry.read_text(encoding='utf-8') + body, sources = changelog_md.convert_traced(text) + count = len(text.split('\n')) + + offsets = [offset for offset, _ in sources] + self.assertEqual(offsets, sorted(offsets), f'{entry.name}: map is unordered') + self.assertTrue(sources, f'{entry.name}: no map at all') + for offset, line in sources: + self.assertTrue( + 0 <= offset < len(body) and 1 <= line <= count, + f'{entry.name}: ({offset}, {line}) is outside a {len(body)}-character ' + f'body of a {count}-line entry', + ) + + class CheckModeTests(ChangelogTreeMixin, unittest.TestCase): """``--check`` is what a CI gate calls, so it has to fail when it should.""" diff --git a/util/changelog_md.py b/util/changelog_md.py index 31068f542..35cd98806 100644 --- a/util/changelog_md.py +++ b/util/changelog_md.py @@ -40,7 +40,9 @@ conversion is six mechanical rules rather than a document converter: 1. the setext version heading becomes an ATX ``##`` heading; - 2. ``:rfc:`NNNN``` becomes a Markdown link to the RFC on the IETF datatracker; + 2. ``:rfc:`NNNN``` -- and ``:rfc:`NNNN#section-3```, the anchored spelling + Sphinx accepts too -- become Markdown links to the RFC on the IETF + datatracker; 3. ``double backtick`` literals become single-backtick code spans; 4. ``*`` bullets become ``-`` bullets; 5. ``[n]_`` / ``.. [n]`` footnotes become GitHub's ``[^n]`` / ``[^n]:``; @@ -94,6 +96,27 @@ Every pattern was measured against all 37 committed entries and matches none of them, so the guard costs nothing until an entry actually leaves the subset. +Where a complaint points +------------------------ + +A complaint cites a line of the **entry file**, because that is the only frame a +reader can act on, and the guard is the message on a gate that blocks merges. + +It is not the frame the guard finds the construct in. Rule 6 joins a wrapped +bullet onto one line, so the converted body is a fraction of the file's length +and a number counted in it is not a location at all: measured on the 1.5.0 +entry, the body is 55 lines against the file's 580, and four roles written on +source lines 467 and 468 all reported as ``line 46``. The bound alone settles it +-- a number that cannot exceed 55 cannot name a line in a 580-line file. + +So :func:`convert_traced` returns a *source map* alongside the Markdown, saying +which line of the entry each stretch of output came from, and :func:`residual` +resolves every hit through it. That is also what separates the hits: several +constructs joined onto one output line become several complaints at their own +source lines, in file order, rather than several copies of one wrong number. +Masking a code span preserves its length for the same reason -- so that an offset +in the masked text is still an offset in the Markdown. + Usage ----- @@ -137,6 +160,22 @@ #: URL instead. RFC_URL = 'https://datatracker.ietf.org/doc/html/rfc' +#: Sphinx's ``:rfc:`` role, in both spellings it accepts: a bare number, and a +#: number with a section anchor -- ``:rfc:`6554#section-3```, which is what +#: anyone writing about a specific section reaches for. Only the anchor shapes +#: :func:`rfc_link` can name are matched; a stranger one falls through to +#: :func:`residual` and is reported, rather than being carried into a link whose +#: text nobody checked. +_RFC_ROLE = re.compile(r':rfc:`(\d+)(?:#([\w.-]+))?`') + +#: Anchor prefixes Sphinx renders as words rather than as part of the target, +#: from ``sphinx.roles._format_rfc_target`` (read from Sphinx 9.1.0, the version +#: this repository builds its documentation with): ``#section-3`` is titled +#: ``Section 3``, and an anchor of any other shape is left exactly as written. +#: Kept level with Sphinx so the Markdown link and the rendered documentation say +#: the same thing about the same page, which is the whole point of rule 2. +_RFC_ANCHORS = frozenset({'appendix', 'page', 'section'}) + #: Where the full rendered history lives. Must match ``[project.urls].changelog`` #: in ``pyproject.toml`` -- ``MANIFEST.in`` prunes ``docs/``, so in a source #: distribution this is the only route from the shipped entry to the rest of the @@ -160,15 +199,32 @@ #: markup: the 1.5.0 entry says ``a Sphinx-only ``:mod:`` role``, which converts #: to the code span ```:mod:``` and must not be read as a role that escaped rule #: 2. Deliberately cannot match across a newline, so masking leaves every line -#: break -- and therefore every reported line number -- where it was. +#: break where it was. _CODE_SPAN = re.compile(r'`[^`\n]*`') -#: Stands in for a masked code span. A *marker* rather than nothing, because the -#: difference between a leftover role and prose about a role is precisely whether -#: a code span follows the ``:name:`` or encloses it: ``:mod:`pcapkit.const``` is -#: a role that escaped rule 2, and ```:mod:``` is a sentence mentioning one. +#: Stands in for one character of a masked code span. A *marker* rather than +#: nothing, because the difference between a leftover role and prose about a role +#: is precisely whether a code span follows the ``:name:`` or encloses it: +#: ``:mod:`pcapkit.const``` is a role that escaped rule 2, and ```:mod:``` is a +#: sentence mentioning one. +#: +#: A span is masked character for character rather than collapsed to one marker, +#: so that a match offset in the masked text is also an offset in the Markdown -- +#: which is what lets :func:`residual` resolve a hit through the source map. No +#: pattern below counts markers, so the runs change nothing about what matches. _SPAN = '\x00' +#: A run of markers, rendered back as one code span when a complaint quotes what +#: it matched. +_SPAN_RUN = re.compile(_SPAN + '+') + +#: A ``double backtick`` literal rule 3 did not reach -- because it holds a +#: backtick, or is empty, or its closing pair was on the next source line. Whole +#: literal where one line holds both pairs, lone pair otherwise, so one leftover +#: literal is one complaint rather than two. Checked before masking: masking +#: would eat ``````` as an empty code span and hide exactly this case. +_LITERAL = re.compile(r'``[^\n]*?``|``') + #: reStructuredText that should be gone by the time conversion finishes. Checked #: against the masked text, and each one means a rule did not fire. Every pattern #: matches none of the 37 committed entries; see the module docstring for what is @@ -213,7 +269,9 @@ class ResidualMarkupError(RuntimeError): Deliberately fatal. The six rules cover the subset the entries are written in; anything outside it would otherwise be copied through as literal text and render as itself in a release body, which is a silent defect in a published - artefact. Failing here instead names the construct and the line. + artefact. Failing here instead names the construct, and the line of the entry + file it was written on -- see "Where a complaint points" in the module + docstring for why that is not the line the guard found it on. """ @@ -285,6 +343,37 @@ def newest(index: pathlib.Path = INDEX) -> tuple[str, pathlib.Path]: return entry.stem, entry +def rfc_link(number: str, anchor: str = '') -> str: + """Render one ``:rfc:`` role as a Markdown link. + + The link text follows Sphinx's own ``:rfc:`` role rather than being invented + here, so an entry reads the same in the generated Markdown as it does in the + rendered documentation: ``:rfc:`6554#section-3``` is *RFC 6554 Section 3* in + both, pointing at the same anchor on the same page. + + Args: + number: The RFC number, as written in the role. + anchor: The fragment after ``#``, if the role carried one. + + Returns: + A Markdown inline link. + + """ + if not anchor: + return f'[RFC {number}]({RFC_URL}{number})' + + # ``section-3`` -> ``Section 3``, as ``sphinx.roles._format_rfc_target`` does. + # An anchor whose prefix Sphinx does not know is shown as written, there as + # here, and a prefix with nothing after it -- ``#section`` -- keeps the word + # alone rather than gaining a trailing space. + kind, _, remaining = anchor.partition('-') + if kind in _RFC_ANCHORS: + title = f'RFC {number} {kind.title()}' + (f' {remaining}' if remaining else '') + else: + title = f'RFC {number}#{anchor}' + return f'[{title}]({RFC_URL}{number}#{anchor})' + + def convert(rst: str) -> str: """Apply the six rules to one per-version entry. @@ -294,9 +383,31 @@ def convert(rst: str) -> str: Returns: The entry as Markdown, ending in a single newline. + """ + return convert_traced(rst)[0] + + +def convert_traced(rst: str) -> tuple[str, list[tuple[int, int]]]: + """Apply the six rules, and record where each piece of the output came from. + + What :func:`convert` returns, plus the bookkeeping :func:`residual` needs to + report a location in *rst* rather than in its own much shorter output. Rules 1 + to 5 rewrite a line in place, so every character of an output line came from + one source line and the map has one entry per line; rule 6 then joins lines, + which is what makes the map necessary at all. + + Args: + rst: The entry's reStructuredText. + + Returns: + The entry as Markdown, ending in a single newline, and its *source map*: + ``(offset, line)`` pairs in ascending *offset* order, each saying that the + Markdown from *offset* onwards was written on 1-based *line* of *rst*. + """ lines = rst.rstrip('\n').split('\n') out = [] # type: list[str] + origin = [] # type: list[int] index = 0 while index < len(lines): @@ -304,14 +415,17 @@ def convert(rst: str) -> str: # 1. setext heading -> ATX. Only ``=`` is used in these files, and only # for the version heading, so the underline can be consumed outright. + # The heading is attributed to the title, not to the underline that + # followed it, because the title is what a reader would look for. if (index + 1 < len(lines) and line and set(lines[index + 1]) == {'='} and len(lines[index + 1]) == len(line)): out.append(f'## {line}') + origin.append(index + 1) index += 2 continue - # 2. the one role these entries use. - line = re.sub(r':rfc:`(\d+)`', lambda match: f'[RFC {match[1]}]({RFC_URL}{match[1]})', line) + # 2. the one role these entries use, in both spellings Sphinx accepts. + line = _RFC_ROLE.sub(lambda match: rfc_link(match[1], match[2] or ''), line) # 3. literals. line = re.sub(r'``([^`]+)``', r'`\1`', line) # 4. bullets, at any indent. @@ -321,12 +435,16 @@ def convert(rst: str) -> str: line = re.sub(r'\[(\d+)\]_', r'[^\1]', line) out.append(line) + origin.append(index + 1) index += 1 - return unwrap(out).rstrip('\n') + '\n' + markdown, sources = unwrap(out, origin) + # ``rstrip`` only ever drops trailing newlines, which no map entry points + # past: the last entry is the last non-blank line's own offset. + return markdown.rstrip('\n') + '\n', sources -def unwrap(lines: Sequence[str]) -> str: +def unwrap(lines: Sequence[str], origin: Sequence[int]) -> tuple[str, list[tuple[int, int]]]: """Rule 6: collapse each paragraph and each bullet onto a single line. A block ends at a blank line, at the next bullet, at a heading, or at a @@ -334,68 +452,127 @@ def unwrap(lines: Sequence[str]) -> str: no literal blocks, no tables, no definition lists -- so joining a block's lines with a single space is lossless. + Joining is also what costs the output its line numbers, so each line's source + is carried alongside it and comes back as the source map described in + :func:`convert_traced`. + Args: lines: The entry's lines, with rules 1 to 5 already applied. + origin: The 1-based source line each of *lines* came from, in step with it. Returns: - The lines with each block joined onto one line. + The lines with each block joined onto one line, and the source map. """ - blocks = [] # type: list[str] - current = [] # type: list[str] + rows = [] # type: list[list[tuple[str, int]]] + current = [] # type: list[tuple[str, int]] def flush() -> None: if current: - blocks.append(' '.join(item.strip() for item in current)) + rows.append(current.copy()) current.clear() - for line in lines: + for line, source in zip(lines, origin): if not line.strip(): flush() - blocks.append('') + # One blank row per run: the flush cycle can leave several behind, and + # a run of blank lines is one paragraph break however long it is -- and + # a run before the first block is no break at all, where the ``\n{3,}`` + # substitution this replaces left an empty first line behind. + if rows and rows[-1]: + rows.append([]) continue if re.match(r'^\s*- ', line) or line.startswith('## ') \ or re.match(r'^\[\^\d+\]: ', line): flush() - current.append(line) + current.append((line, source)) continue - current.append(line) + current.append((line, source)) flush() - # Collapse the runs of blank lines the flush cycle can leave behind. - return re.sub(r'\n{3,}', '\n\n', '\n'.join(blocks)) - - -def residual(markdown: str) -> list[str]: + out = [] # type: list[str] + sources = [] # type: list[tuple[int, int]] + offset = 0 + + for index, row in enumerate(rows): + if index: + out.append('\n') + offset += 1 + for position, (line, source) in enumerate(row): + text = (' ' if position else '') + line.strip() + # The joining space belongs to neither line, and is attributed to the + # one after it. Nothing can match starting there in any case -- the + # line-anchored patterns only ever match at the start of a row, where + # there is no joining space -- but it is the kind of off-by-one that is + # invisible until it is written down. + sources.append((offset, source)) + out.append(text) + offset += len(text) + + return ''.join(out), sources + + +def residual(markdown: str, + sources: Optional[Sequence[tuple[int, int]]] = None) -> list[str]: """Report reStructuredText left in *markdown* that the six rules did not convert. - See the module docstring for the guarded set, and for the two constructs left - deliberately unguarded. + See the module docstring for the guarded set, for the two constructs left + deliberately unguarded, and for why a complaint's line number belongs to the + entry file rather than to *markdown*. Args: markdown: One converted entry, as :func:`convert` returned it. The generated file's trailer is not part of this and does not need to be: :func:`render` checks the body before appending it. + sources: The source map :func:`convert_traced` returned beside *markdown*. + With one, each complaint cites the line of the entry the construct was + written on -- the line a reader can open. Without one there is nothing + to map through, so the complaints count lines of *markdown* and say + so, rather than passing an unusable number off as a location. Returns: - One human-readable complaint per leftover construct, empty when clean. + One human-readable complaint per leftover construct, in source order, + empty when clean. """ - problems = [] # type: list[str] - - # Checked before code spans are masked: masking would eat ``````` as an empty - # code span and hide exactly the case this looks for. - for number, line in enumerate(markdown.split('\n'), 1): - if '``' in line: - problems.append(f'line {number}: an unconverted `` literal: {line.strip()[:70]!r}') - - masked = _CODE_SPAN.sub(_SPAN, markdown) + offsets = [offset for offset, _ in sources] if sources is not None else [] + + def locate(offset: int) -> int: + """The 1-based line *offset* should be reported against.""" + if sources is None: + return markdown.count('\n', 0, offset) + 1 + # A linear scan: the map holds one entry per line of one changelog entry, + # and a clean entry has nothing to locate, so a search is not worth an + # import. ``offsets`` ascends, so the last entry at or before *offset* + # owns it. The first row is never blank, so entry zero sits at offset zero + # and the loop always fires; the default is for a map that is empty. + line = 1 + for index, start in enumerate(offsets): + if start > offset: + break + line = sources[index][1] + return line + + frame = 'line' if sources is not None else 'converted line' + problems = [] # type: list[tuple[int, str]] + + def report(offset: int, label: str, found: str) -> None: + line = locate(offset) + problems.append((line, f'{frame} {line}: {label}: {found!r}')) + + # Checked before code spans are masked; see :data:`_LITERAL`. + for match in _LITERAL.finditer(markdown): + report(match.start(), 'an unconverted `` literal', match.group(0)[:70]) + + masked = _CODE_SPAN.sub(lambda match: _SPAN * len(match.group(0)), markdown) for pattern, label in _RESIDUAL: for match in pattern.finditer(masked): - number = masked.count('\n', 0, match.start()) + 1 - found = match.group(0).replace(_SPAN, '`...`') - problems.append(f'line {number}: {label}: {found!r}') - return problems + report(match.start(), label, _SPAN_RUN.sub('`...`', match.group(0))) + + # In source order, so a reader walks the entry once rather than once per + # pattern; :func:`sorted` is stable, so hits sharing a line keep the order the + # patterns found them in. + return [complaint for _, complaint in sorted(problems, key=lambda item: item[0])] def render(index: pathlib.Path = INDEX) -> str: @@ -413,9 +590,11 @@ def render(index: pathlib.Path = INDEX) -> str: """ _, entry = newest(index) - body = convert(entry.read_text(encoding='utf-8')) + body, sources = convert_traced(entry.read_text(encoding='utf-8')) - problems = residual(body) + # With the map, every complaint cites a line of *entry* -- the file named in + # the message -- so the reader can open one at the number they were given. + problems = residual(body, sources) if problems: raise ResidualMarkupError( f'{entry} uses reStructuredText the six conversion rules do not cover, '