From 068192625c07c1c6605f448daf7994b821545d94 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 2 Oct 2026 18:06:33 -0400 Subject: [PATCH] docs(sphinx): add opt-in issue, pr and discussion roles, and convert the conventions pages Closes nothing yet; the first tranche of #989. A bare ``#NNN`` renders as literal text, so every tracker citation in the built documentation is unclickable. The roles are deliberately opt-in per site rather than an automatic ``#NNN`` rule, because no digit-keyed pattern separates a citation from a packet-diagram label. ``#\d{3}`` already misses 17 two-digit citations that are real, and ``#\d+`` would catch the 45 one-digit RFC diagram labels in pcapkit/protocols/internet/hip.py and pcapkit/protocols/transport/sctp.py (``DH GROUP ID #1``, ``Gap Ack Block #1``), which reference nothing. A role nobody writes cannot corrupt them. ``:issue:`` and ``:pr:`` both caption ``#%s``, so converting a bare citation or an explicit link changes the markup and not the rendered text. Six sites were inline literals and are the exception: ``#934``, ``#949`` and ``#911`` in documentation.rst and ``#759``, ``#805`` and ``#775`` in process.rst rendered as monospace and now render as links in body font. They are separate roles because the issue-versus-pull-request distinction is itself a documented convention and one role would flatten it in the source. ``:discussion:`` exists because the repository has five GitHub Discussions -- 105, 106, 127, 251 and 274 -- for which the issues API returns 404, so ``:issue:`` would render a dead link. Scope, measured rather than taken from the issue: 895 bare citations sit on the surface Sphinx actually renders, which is docs/**/*.rst plus pcapkit/ docstrings and ``#:`` comments. All 1,580 autodoc directives target pcapkit.* and there is no literalinclude, so tests/, util/ and examples/ never reach a page and their citations cannot fail to resolve. 420 of the 895 are actionable; the other 475 are under docs/source/changelog, which #657 owns and util/changelog_md.py generates. This tranche converts 78 sites across the seven conventions pages -- 45 explicit links collapsed, 6 inline literals, 27 bare. All 33 distinct numbers were resolved in one GraphQL issueOrPullRequest call and every one is an issue, so :issue: is correct at each site; the collapsed links asserted label == URL number, so every rendered URL is byte-identical to before. Two tests pinned the bare ``#NNN`` source form and this change reddens both. test_conventions_doc_claims.py's floor now counts explicit links and role citations together, keeping the pairwise label-versus-URL comparison over whatever explicit links remain. test_sentinel_exports_unit.py accepts the citation in either markup form. Both were checked against the old page, where the pre-change assertions fail, so neither is vacuous. Verified: Sphinx 9.1.0 builds with exit code 0, the documented root printed and confirmed inside the worktree, 78 rendered /issues/NNN anchors matching the 78 conversions, and no warning naming any conventions page. tests/project/ gives 258 passed, 1 skipped, 859 subtests; the page-reading corekit tests give 61 passed, 129 subtests. All exit codes read from the process. The conf.py comment also had three inaccuracies of its own, which a change about citation accuracy should not ship: it named two files for the 45 one-digit diagram labels where they live in three (internet/hip.py 36, transport/sctp.py 7, schema/internet/hip.py 2); it said ``#\d{3}`` misses four-digit numbers "now arriving" when there are none yet, and omitted the 17 two-digit ones it does miss; and it claimed rendering was unchanged without the inline-literal exception. The Sphinx build is unchanged against main: 61 warnings and 2 errors on both, no warning or error naming any conventions page, and no warning kind new to the branch. Both errors are pre-existing, in pcapkit/corekit/infoclass.py and pcapkit/protocols/schema/schema.py docstrings, neither of which this change touches. --- docs/source/conf.py | 33 +++++++++++ .../conventions/documentation.rst | 42 +++++++------- .../extension-header-subclassing.rst | 10 ++-- .../source/contributing/conventions/index.rst | 2 +- .../conventions/mint-criterion.rst | 8 +-- .../contributing/conventions/process.rst | 44 +++++++-------- .../conventions/registry-protocol.rst | 56 +++++++++---------- .../conventions/sentinel-convention.rst | 20 +++---- tests/corekit/test_sentinel_exports_unit.py | 12 +++- tests/project/test_conventions_doc_claims.py | 30 ++++++---- 10 files changed, 154 insertions(+), 103 deletions(-) diff --git a/docs/source/conf.py b/docs/source/conf.py index b8ef020e3..3f050af1c 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -69,6 +69,7 @@ # earns a warning. The typehint rendering comes from the third-party # ``sphinx_autodoc_typehints`` below. 'sphinx.ext.autodoc', + 'sphinx.ext.extlinks', 'sphinx.ext.napoleon', 'sphinx.ext.todo', @@ -81,6 +82,38 @@ 'sphinxcontrib.mermaid', ] +# Opt-in roles for the tracker references that the prose cites constantly. A bare +# ``#NNN`` renders as literal text -- nothing in Sphinx resolves it -- so every +# citation in a docstring or an ``.rst`` page is unclickable in the built docs. +# +# These are deliberately opt-in per site rather than an automatic ``#NNN`` rule, +# because no digit-keyed pattern can tell a citation from a packet-diagram label: +# ``#\d{3}`` will miss four-digit numbers once the tracker reaches them -- there are +# none yet -- and already misses 17 two-digit ones, while ``#\d+`` catches the 45 +# one-digit RFC diagram labels across ``pcapkit/protocols/internet/hip.py`` (36), +# ``pcapkit/protocols/transport/sctp.py`` (7) and +# ``pcapkit/protocols/schema/internet/hip.py`` (2) -- ``DH GROUP ID #1``, +# ``Gap Ack Block #1`` and the like -- which are not references to anything. A role +# nobody writes cannot corrupt them. +# +# The caption is ``#%s`` for both ``:issue:`` and ``:pr:`` so that converting a bare +# citation or an explicit link changes the markup and not the rendered text. A +# citation written as an inline literal is the one exception: it rendered as monospace +# and now renders as a link in body font. The two roles exist separately because the +# issue-versus-pull-request distinction is itself a documented convention (see +# ``contributing/conventions/documentation.rst``), and a single role would flatten it +# in the source even though GitHub redirects between ``/issues/NNN`` and ``/pull/NNN`` +# either way. +# +# ``:discussion:`` is needed because a handful of cited numbers are GitHub +# Discussions rather than issues -- the issues API returns 404 for them, so +# ``:issue:`` would link to a page that does not exist. +extlinks = { + 'issue': ('https://github.com/JarryShaw/PyPCAPKit/issues/%s', '#%s'), + 'pr': ('https://github.com/JarryShaw/PyPCAPKit/pull/%s', '#%s'), + 'discussion': ('https://github.com/JarryShaw/PyPCAPKit/discussions/%s', '#%s'), +} + intersphinx_mapping = { 'python': ('https://docs.python.org/3', None), 'dictdumper': ('https://dictdumper.jarryshaw.me/en/latest/', None), diff --git a/docs/source/contributing/conventions/documentation.rst b/docs/source/contributing/conventions/documentation.rst index 643c825c2..34fbb8085 100644 --- a/docs/source/contributing/conventions/documentation.rst +++ b/docs/source/contributing/conventions/documentation.rst @@ -10,7 +10,7 @@ reStructuredText under :file:`docs/source/` and the :mod:`pcapkit` docstrings th reference renders from, since the owner named both when settling the first of these. Nearly all of it was settled on -`#719 `__, the prose sweep, whose +:issue:`719`, the prose sweep, whose thread was the only place most of it lived. As on :ref:`process`, every ruling here is **paraphrased rather than quoted**, on the owner's standing instruction there; the issue named beside a rule is where the original wording is. @@ -20,12 +20,12 @@ Heading Case and Shape **Title Case, and short.** Sentence case belongs only to a heading that genuinely is a sentence -- a how-to question is the example the owner gave -- and that was ruled rare: -a sentence should generally not be used as a title at all. Settled on #719. +a sentence should generally not be used as a title at all. Settled on :issue:`719`. Title Case here is the conventional kind rather than every-word capitalisation. The short function words ``a``, ``an``, ``the``, ``and``, ``or``, ``of``, ``in``, ``for`` and ``to`` stay lowercase unless they lead, which was the reading put to the owner on -#719 and left standing. It is also what the tree does: *The* ``all`` *Extra* and +:issue:`719` and left standing. It is also what the tree does: *The* ``all`` *Extra* and *Issue and Pull Request Labels* on :ref:`process` are both in it. **The casing is the easy half.** A heading can be in Title Case already and still @@ -80,7 +80,7 @@ the page for a string no longer on it. Nothing in CI catches a reference a rename left behind. :file:`docs/source/conf.py` sets no ``nitpicky`` and :file:`docs/Makefile` leaves ``SPHINXOPTS`` empty, so the build runs with neither ``-n`` nor ``-W``: a dead reference renders as the plain text -it used to be, and the build still succeeds. ``#934`` found sixteen of them at once +it used to be, and the build still succeeds. :issue:`934` found sixteen of them at once that way. One thing a rename breaks that no tool checks at all is the prose around it. Turning a @@ -92,17 +92,17 @@ Mermaid for Flows Where the subject is a flow, prefer a Mermaid graph to the paragraph or the ASCII diagram that would otherwise carry it -- a graph is read faster than its own -description. The owner ruled this on #719, asking for it where it is necessary and +description. The owner ruled this on :issue:`719`, asking for it where it is necessary and helpful, which bounds it in three directions: * **A short sequence does not earn a graph.** Two steps read perfectly well as a sentence, and a diagram of them costs a reader a context switch for nothing. * **A rationale stays prose.** A diagram carries structure and sequence; it cannot - carry *why* a choice was made, and that reasoning is what #719 protects rather than - compresses. + carry *why* a choice was made, and that reasoning is what :issue:`719` protects rather + than compresses. * **Do not redraw a graph another page already has.** The owner's condition when - approving the navigation work on #719 was that nothing duplicate information already - shown, and a second copy of a flow is exactly that. + approving the navigation work on :issue:`719` was that nothing duplicate information + already shown, and a second copy of a flow is exactly that. The style model is the set already in the tree, every one of which builds. The sweep excludes this page, which writes the directive name three times in its own prose and @@ -163,7 +163,7 @@ count itself: Those root toctrees are ``:hidden:`` because, without it, each of the three captions rendered twice on the root page -- once inline in the body, once in the sidebar -- which -is the duplication the owner ruled out on #719. +is the duplication the owner ruled out on :issue:`719`. .. note:: @@ -182,8 +182,8 @@ Paraphrasing a Ruling ~~~~~~~~~~~~~~~~~~~~~ Write a ruling down in your own words. **Do not quote the owner verbatim** -- a -standing instruction on #719, and the one every page in this directory follows. -``#949`` went back over the five pages that then existed and replaced their quoted +standing instruction on :issue:`719`, and the one every page in this directory follows. +:issue:`949` went back over the five pages that then existed and replaced their quoted rulings with paraphrase. What a quotation costs is not style. A quoted sentence is pinned to the moment it was @@ -198,7 +198,7 @@ describes what was true on the day it merged, and the next change past it can ma citation wrong without touching it. The issue is the durable half -- where the ruling was asked for and given -- and it survives the work that implemented it. So cite the issue a rule was settled on, and describe a change by what it did rather than by its -number. Ruled on #719. +number. Ruled on :issue:`719`. **The changelog and** :file:`tests/` **are both exempt, for related reasons.** A changelog entry exists so a reader can find the change, and the pull-request number *is* @@ -207,7 +207,7 @@ that pointer; converting it would delete the thing the entry is for -- A substantial share of the pull requests cited under :file:`tests/` close no issue at all -- one credits a proposal to an external contributor and closes nothing -- and where an issue does exist beside a citation, it frequently lacks the fact being cited, which -lives in the pull request's own body or review thread instead. Ruled on #719. +lives in the pull request's own body or review thread instead. Ruled on :issue:`719`. The rule reaches the rest of this directory as well: a sibling page that cites a pull request is unconverted, not a third exemption. The changelog and :file:`tests/` are the @@ -217,8 +217,8 @@ Accuracy ~~~~~~~~ **Verify a claim against the code it describes, never against another document.** Where -prose and code disagree the code wins and the prose is what gets fixed -- #719's own -charter -- and a docstring outliving the thing it described is a demonstrated failure +prose and code disagree the code wins and the prose is what gets fixed -- :issue:`719`'s +own charter -- and a docstring outliving the thing it described is a demonstrated failure mode here rather than a hypothetical one. **Re-derive a count; do not copy one.** Better still, write down the command that @@ -230,14 +230,14 @@ merge base, and re-measures the intersection. A tense-keyword grep misses a clai phrased as a fraction of a total. **Treat** ``every``, ``all``, ``each`` **and** ``none`` **as a claim about members, and -check the members one at a time.** Several of #719's findings were of exactly that +check the members one at a time.** Several of :issue:`719`'s findings were of exactly that shape: * A sweep asserted that every ``.. module::`` target in the documentation resolved. One did not: :file:`docs/source/pcapkit/protocols/link/rarp.rst` declared ``pcapkit.protocols.data.link.rarp``, which has never existed, because RARP and DRARP reuse ARP's data class. Fixed in ``68fbccd90``. -* ``#911``'s ruling -- export the sentinel objects and leave their types out -- was +* :issue:`911`'s ruling -- export the sentinel objects and leave their types out -- was read as describing all three modules that then held a sentinel. One ran the other way: :mod:`pcapkit.corekit.fields.field` exported neither, so applying the rule there meant *adding* a name rather than removing one. @@ -261,7 +261,7 @@ Resolvable Targets **A** ``.. module::`` **target must name a file on disk.** A dangling one is worse than no directive at all: it registers a module-index entry for a module that does not exist, and gives cross-references a target that resolves to nothing. The sweep that -settled this on #719 found exactly one, and repeating it is cheap: +settled this on :issue:`719` found exactly one, and repeating it is cheap: .. code-block:: shell @@ -285,13 +285,13 @@ omits all of them. When something is removed, its documentation entry goes with it -- carrying a deprecation note where a user could have depended on the thing, and deleted outright where it never shipped. The ``rarp`` entry above needed no note for that second reason. -Asked and ruled on #719. +Asked and ruled on :issue:`719`. Format and Mechanics ~~~~~~~~~~~~~~~~~~~~ * **reStructuredText under** :file:`docs/source/`, **Markdown outside it.** The owner - ruled this on #719, correcting a blanket *always* ``.rst`` that had been in + ruled this on :issue:`719`, correcting a blanket *always* ``.rst`` that had been in circulation until then: the Sphinx documentation is reST, and the other documents -- the READMEs included -- are Markdown where that applies. ``CONTRIBUTING.md``'s own *Documentation* section records the same split. One trap arrived with the ruling: diff --git a/docs/source/contributing/conventions/extension-header-subclassing.rst b/docs/source/contributing/conventions/extension-header-subclassing.rst index 3da71408c..e4186a387 100644 --- a/docs/source/contributing/conventions/extension-header-subclassing.rst +++ b/docs/source/contributing/conventions/extension-header-subclassing.rst @@ -7,7 +7,7 @@ Every IPv6 extension header in this package subclasses :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`. Some name a **second** base as well, and which ones do is a ruling rather than an accident. The owner ruled, in review of the rename that made ``IPv6_Ext`` the shared base -(`#917 `__), that a header usable +(:issue:`917`), that a header usable *only* as an extension header inherits ``IPv6_Ext`` and nothing else -- ``IPv6_Frag`` being the example -- while one that is usable as a standalone protocol in its own right inherits both ``IPv6_Ext`` and ``Internet`` (or ``IPsec``), as ``ESP`` does. @@ -106,7 +106,7 @@ class for it, so nothing implements the classification, but a future one inherit Own-protocolhood on its own is **not** sufficient, and MH is the case that settles it: the alternative reading -- that a protocol in its own right qualifies whether or not it can appear under IPv4 -- was put to the owner explicitly in - review of `#917 `__ and not + review of :issue:`917` and not taken, so MH and ``Shim6`` stay extension-only. A header that is a protocol in its own right but structurally cannot be an IPv4 payload names ``IPv6_Ext`` alone. @@ -133,8 +133,8 @@ The Retired ``IPv6_GenericExt`` Name ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The class arrived as ``IPv6_GenericExt``, a fallback parser for an unrecognised -extension header (`#891 `__), and -`#917 `__ merged that role with +extension header (:issue:`891`), and +:issue:`917` merged that role with the shared-base role into one class under the shorter name. No compatibility alias was left behind, and that was deliberate. The owner ruled that the ``IPv6_GenericExt`` name goes for good: it was an intermediate state, and it was never @@ -160,7 +160,7 @@ Payload (ESP) is not considered an extension header"* -- but that sentence opens *"For this purpose,"*, scoping it to the fragmentation discussion it sits in, and the sentence after it lists ESP among *"examples of upper-layer headers"*. The library follows the registry, on the owner's ruling for -`#895 `__, which is why ESP +:issue:`895`, which is why ESP carries the same extension-mode contract as its siblings. **And it terminates the chain walk.** :rfc:`4303` places ESP's Next Header byte diff --git a/docs/source/contributing/conventions/index.rst b/docs/source/contributing/conventions/index.rst index 79448aec0..dbfe01ec0 100644 --- a/docs/source/contributing/conventions/index.rst +++ b/docs/source/contributing/conventions/index.rst @@ -22,7 +22,7 @@ House Conventions the page that covers it in the same change that implements it, rather than left in the issue for the next contributor to find. That is the owner's standing ask on - `#918 `__. + :issue:`918`. .. toctree:: :maxdepth: 1 diff --git a/docs/source/contributing/conventions/mint-criterion.rst b/docs/source/contributing/conventions/mint-criterion.rst index 3cb73e554..95eb74d0b 100644 --- a/docs/source/contributing/conventions/mint-criterion.rst +++ b/docs/source/contributing/conventions/mint-criterion.rst @@ -50,8 +50,8 @@ the label as the final, concrete assigned name, or only as a notation for a huma reading the table? Settled in review of the ``Socket._missing_`` branch-order fix -(`#841 `__) and reaffirmed -on `#775 `__ as a core concept of +(:issue:`841`) and reaffirmed +on :issue:`775` as a core concept of the ruling. So the question to ask of a range is **what the upstream registry actually did**, not @@ -98,9 +98,9 @@ Suffixed Company Names ~~~~~~~~~~~~~~~~~~~~~~ The ethertype case looks like an exception to the rule and is not. The maintainer's -reasoning, settled on `#775 `__ after +reasoning, settled on :issue:`775` after being raised in review of the same ``Socket._missing_`` fix -(`#841 `__): a proprietary protocol +(:issue:`841`): a proprietary protocol will never have a public name, so the company name is what serves that purpose in its place. diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index 817b8ca87..faa96d77e 100644 --- a/docs/source/contributing/conventions/process.rst +++ b/docs/source/contributing/conventions/process.rst @@ -7,7 +7,7 @@ The four pages before this one are about writing library code. The rulings here about running the repository -- what an install carries, what a changelog entry is, and what the issue and pull request labels mean. None of them is derivable from a module, and none fits a code-convention page, so the owner ruled on -`#918 `__ that they get a page of +:issue:`918` that they get a page of their own rather than being left in their threads. Each ruling below is **paraphrased rather than quoted**, also on the owner's standing instruction there; the issue named beside it is where the original wording is. @@ -17,7 +17,7 @@ The ``all`` Extra ``all`` means **core addons only** -- the things that let the library itself run at full functionality -- rather than everything a user might conceivably want. The owner -settled that on `#910 `__ and named +settled that on :issue:`910` and named the three that qualify to date: the CLI addon, the crypto addon, and ``pycrate``. On the tree, in :file:`pyproject.toml`: @@ -68,7 +68,7 @@ Changelog Entry Granularity An entry is **not one line per commit**. Group the changes by topic, and give concise detail of what actually changed in that version bump. Ruled on -`#918 `__. +:issue:`918`. Two different things get confused here, so they are named apart: @@ -98,7 +98,7 @@ commands down instead of a figure that will be stale by the next merge: --search 'shared 1.5.0 changelog in:title' \ --json commits -q '.[].commits|length' -The grouping scheme was settled on #918: **a section per top-level module, with** +The grouping scheme was settled on :issue:`918`: **a section per top-level module, with** ``Added``/``Changed``/``Fixed`` **nested inside each** -- module granularity, not per-file and not per-subpackage. The file carries **9** module-level sections holding 155 entries, and no entry carries an inline kind label:: @@ -121,11 +121,11 @@ belongs to none. Nor is the map one-to-one with the package list below -- One case the rule does not settle by itself: an entry whose change spans modules -- the reassembly and extraction ones touch :mod:`pcapkit.foundation` and :mod:`pcapkit.protocols` together. Ruled on - `#952 `__: file it under the + :issue:`952`: file it under the module the change is *about*, name the others in the entry's own text, and do **not** duplicate the entry into each section. A reader scanning one module's section wants that module's changes; the same prose appearing twice reads as two - separate changes. Raised originally on #918. + separate changes. Raised originally on :issue:`918`. The restructure itself belongs to the shared changelog's own pull request, which owns the file and merges last; doing it earlier would conflict with every open @@ -134,7 +134,7 @@ belongs to none. Nor is the map one-to-one with the package list below -- Issue and Pull Request Labels ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -The owner asked on `#918 `__ for +The owner asked on :issue:`918` for this to be written down alongside ``breaking``, since ``breaking``'s meaning only makes sense against the scheme it sits in. @@ -174,12 +174,10 @@ this page does not go stale every time one is added:: One of those defaults carries a local ruling worth knowing: an issue closed as unnecessary takes ``invalid`` (or the nearest applicable) rather than ``bug``, since -the issue was not a defect. `#275 -`__ is where it was applied -- -``bug`` removed and ``invalid`` added in the same second -- and `#707 -`__ is the worked example, closed -as invalid because it was filed against ``main`` rather than against the pull -request's diff. +the issue was not a defect. :issue:`275` is where it was applied -- +``bug`` removed and ``invalid`` added in the same second -- and :issue:`707` is the worked +example, closed as invalid because it was filed against ``main`` rather than against the +pull request's diff. **Type -- what kind of change it is.** Each corresponds to the subject prefix of the commit, so the label and the message agree by construction: @@ -228,7 +226,7 @@ these, so that its status is readable without opening it: Two things the board shows rather than the rule: ``wip`` and ``needs: decision`` legitimately **co-occur**, when the bulk of an issue is being worked and one -sub-question is held for the owner -- #918 itself was labelled that way while this +sub-question is held for the owner -- :issue:`918` itself was labelled that way while this page was being written. And an open issue with no state label at all is a gap rather than a category, which is worth checking for rather than assuming away: @@ -261,18 +259,18 @@ observe the difference without changing their code"**. On the tree, the changes carrying it are that kind: * an exception type a caller catches -- - `#805 `__ raising + :issue:`805` raising :exc:`~pcapkit.utilities.exceptions.ProtocolError` where a bare :exc:`struct.error` used to escape, and - `#759 `__ raising one where a + :issue:`759` raising one where a single-bit lookup used to return a member; * a public attribute's meaning -- - `#618 `__ swapping ``Frame.len`` + :issue:`618` swapping ``Frame.len`` and ``Frame.cap_len`` between the PCAP and PCAP-NG readers; * a signature or a name a caller writes -- - `#806 `__ retyping + :issue:`806` retyping ``AppType.proto`` and giving ``register_apptype`` varargs, - `#778 `__ enforcing ``@final`` + :issue:`778` enforcing ``@final`` at runtime; * a path a caller or a script depends on -- the change that named the examples directories apart. @@ -280,10 +278,10 @@ carrying it are that kind: .. warning:: **A pull request's prose and its label can disagree, and the label is not - automatically right.** Both directions have happened here. The ``#759`` and - ``#805`` changes carry the label while their changelog bullets never said so, which + automatically right.** Both directions have happened here. The :issue:`759` and + :issue:`805` changes carry the label while their changelog bullets never said so, which a review round on the shared changelog caught and corrected. The - `#844 `__ change carries it too, + :issue:`844` change carries it too, and its own pull request argues at length that the change is *not* breaking -- a review round checked that argument and found it right on the facts, so there the label is the half that overstates. So when the two conflict, settle it on what a @@ -299,7 +297,7 @@ carrying it are that kind: of the numbering, with nothing labelled at all between them and that change. Three of those seven are distribution rollups, each also carrying ``release``; the other four are early ``refactor``/``feat`` work from before the project stabilised. On - issues it is sparser still, appearing only from ``#775`` up. So ``breaking``'s + issues it is sparser still, appearing only from :issue:`775` up. So ``breaking``'s absence on an old pull request is weak evidence at best. The current figures, rather than these: diff --git a/docs/source/contributing/conventions/registry-protocol.rst b/docs/source/contributing/conventions/registry-protocol.rst index c9a0f1836..803759b15 100644 --- a/docs/source/contributing/conventions/registry-protocol.rst +++ b/docs/source/contributing/conventions/registry-protocol.rst @@ -8,7 +8,7 @@ The Registry Protocol :meth:`~pcapkit.corekit.enum.EnumRegistry.register` and :meth:`~pcapkit.corekit.enum.EnumRegistry.register_alias` are expected to exist on **every** registry, per the ruling on -`#842 `__. They come from +:issue:`842`. They come from :class:`~pcapkit.corekit.enum.EnumRegistry`, mixed in ahead of the enum base so that ``_member_type_`` still resolves to :class:`int` or :class:`str`: @@ -24,14 +24,14 @@ share the generated template: :class:`~pcapkit.const.ftp.command.Command`, :class:`~pcapkit.const.http.status_code.StatusCode`, :class:`~pcapkit.const.pcapng.option_type.OptionType` and :class:`~pcapkit.const.reg.apptype.apptype.AppType`. They are on the base regardless -- -`#860 `__ finished that -- so a +:issue:`860` finished that -- so a bespoke ``__new__`` exempts a registry from the template, not from the protocol. The Two-Tier Hierarchy ~~~~~~~~~~~~~~~~~~~~~~ :class:`~pcapkit.corekit.enum.EnumRegistry` is not the only base any more. Since -phase 1 of `#877 `__ it has a +phase 1 of :issue:`877` it has a parent, and the line between them is whether the enumeration may *grow*: =================================================== ============================================================== @@ -40,7 +40,7 @@ parent, and the line between them is whether the enumeration may *grow*: ``_extend``, ``_unregistered_member`` =================================================== ============================================================== -The owner ruled on #877 that a registry may subclass a bare base enum out of +The owner ruled on :issue:`877` that a registry may subclass a bare base enum out of :mod:`pcapkit.corekit.enum`, with :class:`~pcapkit.corekit.enum.EnumRegistry` subclassing that base in turn for use by the mutable ones. So a **closed** set inherits :class:`~pcapkit.corekit.enum.EnumLookup` @@ -104,7 +104,7 @@ Three things about it are easy to get wrong: Re-parenting every non-registry enumeration onto :class:`~pcapkit.corekit.enum.EnumLookup` was **phase 2** of - `#877 `__, and it is now + :issue:`877`, and it is now **complete**: the phase landed for 24 of the 24 non-registry enumerations. **Zero enumerations remain outside the hierarchy**, measured by the same runtime walk over both the :mod:`enum` and ``aenum`` flavours that once found seven. @@ -114,7 +114,7 @@ Failed-Lookup Exceptions Two rules govern which exception a failed lookup raises, and they pull in opposite directions on purpose. The owner ruled on -`#923 `__ that the choice between +:issue:`923` that the choice between :exc:`ValueError` and :exc:`KeyError` follows whichever stdlib's :class:`~enum.Enum` would raise in the same circumstance, and that whichever it is comes from :mod:`pcapkit.utilities.exceptions` rather than from builtins. @@ -152,8 +152,8 @@ Which branch a key takes, and what each miss ends in: VALUE -->|"ValueError, no usable default"| VALERR["EnumValueError
loud, derives ValueError"] Do not "improve" on the shape by making both misses report identically. Converting -one into the other is exactly what #923 retired, and it was retired in three places -at once: ``TransportProtocol.get`` and ``Criticality.get`` had each turned the +one into the other is exactly what :issue:`923` retired, and it was retired in three +places at once: ``TransportProtocol.get`` and ``Criticality.get`` had each turned the base's :exc:`KeyError` into a :exc:`ValueError`, and ``FastBindingAcknowledgmentStatus.get`` raised :exc:`~pcapkit.utilities.exceptions.EnumValueError` for a name miss, so that the two @@ -168,7 +168,7 @@ loud. A name miss is in-library control flow at several call sites, and at that override catches it in order to mint. A loud error there would put a :data:`logging.CRITICAL` record on every such call and set :data:`sys.tracebacklimit` to ``0`` process-wide, which is the -`#362 `__ defect ``quiet`` +:issue:`362` defect ``quiet`` exists for. So a ``get`` override that catches a name miss as control flow is following the convention; one that catches a *value* miss that way is silencing a logged error, and needs a reason. @@ -211,10 +211,10 @@ guaranteed either: pass a first argument that *is* an instance of the class and delegation **silently succeeds**, so a ``@staticmethod`` override cannot even be relied on to fail loudly. Measured, all three cases, rather than reasoned about. :meth:`~pcapkit.corekit.enum.EnumLookup.get` is itself a ``@classmethod``. The -precedent is `#903 `__, which made +precedent is :issue:`903`, which made ``FEATCode.get`` a ``@classmethod def get(cls, key, default=NO_DEFAULT)`` ending in ``return super().get(key, default)``; -`#908 `__ followed it, which is +:issue:`908` followed it, which is what turned ``Method.get`` into a classmethod. Callers cannot see the switch -- ``Method.get('X')`` binds identically either way -- @@ -225,7 +225,7 @@ so there is no compatibility argument for keeping the ``@staticmethod``. Two all, so neither meets the condition. **Raise the way the base raises, which means** ``quiet=True``. -`#933 `__ asked whether two +:issue:`933` asked whether two overrides raising :exc:`~pcapkit.utilities.exceptions.EnumKeyError` **without** ``quiet=True`` should adopt the base's. The owner first declined, then reversed course: they should follow the house convention and not be loud. @@ -234,7 +234,7 @@ Both answers are on the issue deliberately, and the reversal is the ruling. What settles is not the one keyword -- it is the tie-breaker. A loud :class:`~pcapkit.utilities.exceptions.BaseError` sets :data:`sys.tracebacklimit` to ``0`` **process-wide**, the -`#362 `__ hazard, so loudness is +:issue:`362` hazard, so loudness is paid for by the whole library rather than by the override's own callers. The argument against changing them was that ``quiet=True`` exists on the base for a name miss inside a *successful* call at ``Method.get`` and these two had no such caller; @@ -252,7 +252,7 @@ refused:: A ``# type: ignore[override] # pylint: disable=arguments-differ`` pair hid the mismatch from ``mypy`` and ``pylint``, and both docstrings disclosed it in prose instead. Put to the owner on -`#935 `__ as one of three options +:issue:`935` as one of three options -- widen and delegate, refuse ``default`` explicitly with an in-library error, or leave the disclosure as the settled answer -- the first was the ruling. So **a suppression plus a docstring is not an answer to a contract the class advertises @@ -286,7 +286,7 @@ locally-defined helpers in those two modules, and no prose anywhere said so. Re-implementing the dispatch is how an override acquires a divergence nobody wrote down; delegating to it is how it does not. -Taken with the ``Criticality.get`` deletion below -- an override emptied by #923 +Taken with the ``Criticality.get`` deletion below -- an override emptied by :issue:`923` rather than by redundancy -- the rule generalises: **an override justifies itself by what it adds to the base, and goes when the answer is nothing.** @@ -294,7 +294,7 @@ RFC-Directed Case Sensitivity ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The rule, as the owner ruled it on -`#877 `__: where the RFC states the +:issue:`877`: where the RFC states the values are case-insensitive, the enumeration treats them that way too; otherwise it treats them as case-sensitive. @@ -317,11 +317,11 @@ resolvable. And a folding override carries **only** the fold. ``TransportProtocol.get`` is the worked example: since -`#923 `__ it lowers ``key``, +:issue:`923` it lowers ``key``, forwards ``default`` verbatim and delegates to ``super().get()``, and that is all it does. It used to convert the base's name-miss :exc:`KeyError` into a -:exc:`ValueError` as well, and #923's ruling retired that; the -`#808 `__ refusal to extend the +:exc:`ValueError` as well, and :issue:`923`'s ruling retired that; the +:issue:`808` refusal to extend the class at all is untouched by the retirement, since only the exception class moved. ``Criticality.get`` went further and no longer exists: conversion was the *only* thing it added over the base, so once that went there was nothing left for an @@ -335,7 +335,7 @@ The Lenient Criterion, in Two Limbs ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The ruling above leaves one question open, and -`#903 `__ settled it: does a +:issue:`903` settled it: does a specification have to state a **comparison rule** for a registry to be treated case-insensitively, or does it also count when the authorities merely **disagree about spelling**? The owner ruled for the lenient reading, with ``TransportProtocol`` @@ -363,7 +363,7 @@ Where neither limb holds, the lookup is case-sensitive and inherits The Audit, per Class ~~~~~~~~~~~~~~~~~~~~ -`#903 `__'s sweep, so that a +:issue:`903`'s sweep, so that a registry added later has something to check itself against. The owner set its scope: audit every registry first, and decide case-sensitivity per registry from that. @@ -423,7 +423,7 @@ That leaves the classes with something to decide: - :rfc:`9110#section-9.1` - *"The method token is case-sensitive."* Explicitly the opposite of limb 1. - **case-sensitive** -- was a defect, fixed by - `#896 `__ + :issue:`896` * - :class:`~pcapkit.const.pcapng.option_type.OptionType` - ``draft-tuexen-opsawg-pcapng`` - Nothing states a rule; the draft never discusses option-name case. @@ -444,7 +444,7 @@ That leaves the classes with something to decide: all 14,536 rows (``tcp`` 6608, ``udp`` 6357, blank 1467, ``sctp`` 93, ``dccp`` 11, zero upper-case). - **case-insensitive** -- ``get`` folds, and this is the owner's own example. - Folding is now the *only* thing that override adds (#923) + Folding is now the *only* thing that override adds (:issue:`923`) * - :class:`~pcapkit.const.reg.apptype.apptype.AppType` - -- - Moot: its ``get`` takes a port number and refuses a non-:class:`int` outright, @@ -487,8 +487,8 @@ That leaves the classes with something to decide: ``LocalizedRoutingStatus`` never carried a ``get`` at all, so they had no string lookup to fold. ``Criticality`` had one when this audit was taken and no longer does: - `#877 `__ re-parented it onto - :class:`~pcapkit.corekit.enum.EnumLookup` and #923 retired the exception + :issue:`877` re-parented it onto + :class:`~pcapkit.corekit.enum.EnumLookup` and :issue:`923` retired the exception conversion that was the override's only remaining job, so it now inherits ``get`` unchanged. - **case-sensitive** -- conforms @@ -523,7 +523,7 @@ wider than a case fix: crawler translates the CSV's lower-case letters to the upper-case member names at generation time. Both classes inherit :class:`~pcapkit.corekit.enum.EnumLookup`, since - `#930 `__ completed phase 2's + :issue:`930` completed phase 2's re-parenting, so a ``get`` exists on each, case-sensitive like the base's own. Whether to fold case to match ``TransportProtocol``'s own override is a design question for whoever writes the first string-keyed caller, not one this @@ -557,7 +557,7 @@ rather than changing it. The example above is :class:`~pcapkit.const.ftp.command.FEATCode`'s shape, and it still resolves exactly as shown -- but since - `#903 `__ that class overrides + :issue:`903` that class overrides ``get`` too, so the output is only the base's because its override delegates an exact name-or-value hit straight through. Measure the base on a registry that does **not** override ``get`` at all. **Five** do -- @@ -574,7 +574,7 @@ rather than changing it. before matching, which makes it look as though the base were case-insensitive -- deliberately, since :rfc:`959#section-5` treats FTP command codes identically regardless of case. ``Method.get`` used to fold case the same way, but - `#896 `__ made it + :issue:`896` made it case-sensitive instead: :rfc:`9110#section-9.1` says the HTTP method token is case-sensitive, so ``Method.get('get')`` no longer resolves to ``Method.GET`` -- it builds its own unregistered member, preserving the diff --git a/docs/source/contributing/conventions/sentinel-convention.rst b/docs/source/contributing/conventions/sentinel-convention.rst index 0171cfd5d..ec6f0a021 100644 --- a/docs/source/contributing/conventions/sentinel-convention.rst +++ b/docs/source/contributing/conventions/sentinel-convention.rst @@ -10,9 +10,9 @@ the sentinel object's type class is named ``Type``. That is, the class takes the instance's name in CamelCase with ``Type`` appended. It says nothing about the **object**'s own name, which is what let three casings diverge -with no rule naming any of them wrong. GitHub issue #937 closed that gap: the owner -ruled for SCREAMING_SNAKE and accepted the resulting breaking change outright, with no -backport. So the object is named in SCREAMING_SNAKE and the type-naming rule above +with no rule naming any of them wrong. GitHub issue :issue:`937` closed that gap: the +owner ruled for SCREAMING_SNAKE and accepted the resulting breaking change outright, with +no backport. So the object is named in SCREAMING_SNAKE and the type-naming rule above derives from it mechanically -- title-case each underscore-separated word and append ``Type``, no per-sentinel exception needed. The four in the tree follow it: @@ -36,8 +36,8 @@ derives from it mechanically -- title-case each underscore-separated word and ap - ``AbsentType`` - :mod:`pcapkit.corekit.sentinels` -GitHub issue #911's housing ruling -- one module for all four -- is why the table names -a single defining module. Each of the four modules that *uses* a sentinel keeps a +GitHub issue :issue:`911`'s housing ruling -- one module for all four -- is why the table +names a single defining module. Each of the four modules that *uses* a sentinel keeps a re-export of it, so ``from import `` keeps working for :mod:`pcapkit.corekit.module`, :mod:`pcapkit.corekit.fields.field`, :mod:`pcapkit.corekit.enum` and :mod:`pcapkit.protocols.protocol` alike, including a @@ -48,15 +48,15 @@ renaming a published sentinel again costs every caller for no further gain. ``ABSENT`` carries no leading underscore even though it is private -- it is read in ``_declared_keywords`` and discarded there, never leaving -:mod:`pcapkit.protocols.protocol`. The owner ruled on #937 that dropping the underscore -is fine so long as the documentation states that the type and the object are private and -not for public use, which is what this page and +:mod:`pcapkit.protocols.protocol`. The owner ruled on :issue:`937` that dropping the +underscore is fine so long as the documentation states that the type and the object are +private and not for public use, which is what this page and :class:`~pcapkit.corekit.sentinels.AbsentType`'s own docstring do in its place. So **privacy here is documentation-only**, and nothing in the name marks it out: when adding a sentinel, add it to the table above whether or not it is public. -What reaches users is the **object only**. The owner ruled on GitHub issue #911 that -the objects alone -- ``NULL`` and its siblings -- are exported to users, so a public +What reaches users is the **object only**. The owner ruled on GitHub issue :issue:`911` +that the objects alone -- ``NULL`` and its siblings -- are exported to users, so a public sentinel names its instance in its module's ``__all__`` and leaves the type out of it. The type stays importable by its dotted path, for an annotation or an ``is`` guard; it is ``import *`` that no longer offers it. A private sentinel such as ``ABSENT`` is in diff --git a/tests/corekit/test_sentinel_exports_unit.py b/tests/corekit/test_sentinel_exports_unit.py index f78aaedb5..50cc7d980 100644 --- a/tests/corekit/test_sentinel_exports_unit.py +++ b/tests/corekit/test_sentinel_exports_unit.py @@ -406,10 +406,20 @@ def test_conventions_doc_records_that_only_the_object_is_exported(self) -> 'None boundary (``__all__``, an identifier rather than prose) and the issue that settled it. + The citation is accepted in either markup form. The page now writes it with the + ``:issue:`` role configured in ``docs/source/conf.py``, because a bare ``#NNN`` + resolves to nothing in the built documentation -- so pinning the bare spelling + would again redden this test over markup while the ruling it checks is + untouched, which is the same defect the paragraph above records. + """ section = _sentinel_section() - self.assertIn('#911', section) + self.assertTrue( + '#911' in section or ':issue:`911`' in section, + 'the page no longer cites the issue that settled the export boundary, in ' + 'either the bare or the role form, so a reader cannot find the ruling ' + 'behind the rule') self.assertIn('__all__', section, "the page no longer names ``__all__``, so it no longer says " 'where the export boundary is -- #911 ruled that the instance ' diff --git a/tests/project/test_conventions_doc_claims.py b/tests/project/test_conventions_doc_claims.py index 33d8ce3e5..6b4095f48 100644 --- a/tests/project/test_conventions_doc_claims.py +++ b/tests/project/test_conventions_doc_claims.py @@ -1615,23 +1615,33 @@ def test_every_issue_link_number_matches_its_own_url(self) -> 'None': Scans every page rather than one, since a link can land on any of them. + The pages have since moved to the ``:issue:``/``:pr:`` roles configured in + ``docs/source/conf.py``, which is the move the floor below was written to + notice. A role carries no second number, so there is nothing to disagree with + itself -- the mismatch class this check exists for cannot arise there, and + Sphinx errors on a role name it does not know. So the floor now counts both + forms, and the pairwise comparison still runs over whatever explicit links + remain. + """ pattern = re.compile( r'`#(\d+)\s*`__') - found = pattern.findall(_every_page()) + text = _every_page() + found = pattern.findall(text) + roles = re.findall(r':(?:issue|pr|discussion):`(\d+)`', text) # A floor, because `assertEqual([], [])` is what an emptied page produces. This - # check disables itself silently on any link-style change -- a single-underscore - # named reference, or a move to an `:issue:` role, takes the regex to zero - # matches while the docstring goes on claiming it closes the largest class of - # unpinned claim. The module guards this shape five other times; this one had - # been left out. + # check disables itself silently on any citation-style change -- a + # single-underscore named reference, or a further move away from the roles, + # takes both patterns to zero while the docstring goes on claiming it closes + # the largest class of unpinned claim. The module guards this shape five other + # times; this one had been left out. self.assertGreater( - len(found), 40, - f'only {len(found)} issue links matched the pinned form, so this check is ' - 'no longer examining the pages -- the link style changed and the test went ' - 'quiet rather than red') + len(found) + len(roles), 40, + f'only {len(found)} explicit links and {len(roles)} role citations matched ' + 'the pinned forms, so this check is no longer examining the pages -- the ' + 'citation style changed and the test went quiet rather than red') mismatched = [(shown, target) for shown, target in found if shown != target]