Skip to content

docs: sweep all prose for concision and accuracy, and drop timed context outside the changelog #719

Description

@JarryShaw

Sweep the prose across docstrings, code comments, Sphinx .rst, the markdown docs, and the changelog for clarity, accuracy, and — the main point — concision.

Do this last. It touches nearly every file, so it will conflict with anything still open. It should start only once the other open issues are closed and the PR queue is empty.

What to fix

Cut length. Prefer the shortest statement that is still precise. Drop restatement, hedging, and background a reader does not need at that point. A docstring should say what a thing is, what it takes, and what it returns.

Keep design decisions. Concision is not an excuse to delete rationale. Where a choice was deliberate and a reader would otherwise undo it — why this type, why this guard, why this option was rejected — that reasoning stays. Tighten how it is written, do not remove it.

Cut timed context. Documentation should describe what the code does now, not when or why it changed. Remove references to when something was added, which PR or issue changed it, what it used to do, and how long a state has persisted. Version-bounded prose is the exception and stays: changelog entries, breaking-change notes, deprecation windows, and .. versionadded:: / .. versionchanged:: directives.

Keep accuracy. Where prose and code disagree, the code wins; fix the prose. Counts, line references, and type names are the usual offenders and rot silently.

The changelog needs this too

docs/source/changelog/1.5.0.rst entries have grown to paragraph length, several carrying full narrative rationale. They are legitimately time-bound, so the timed-context rule above does not apply to them — but the concision rule does. An entry should say what changed and what the effect is; the investigation behind it belongs in the issue, not the entry.

Root CHANGELOG.md is generated from the .rst by python util/changelog_md.py and must never be hand-edited — shorten the source and regenerate.

Scope

  • pcapkit/** docstrings and comments
  • docs/source/**/*.rst
  • docs/source/changelog/** — concision only, not timed context
  • README.rst, CONTRIBUTING.md, and the other root documents
  • tests/** comments, where they explain intent

Out of scope: root CHANGELOG.md as a direct edit target (regenerate it instead); pcapkit/const/** and pcapkit/vendor/** docstrings generated from IANA registries; verbatim ports of upstream code.

Approach

Split by directory into separate PRs. A single sweep across the whole tree is unreviewable, and the diff will be large even per-directory. The changelog is its own PR, and must not be folded into #657 — that PR is about accuracy and is close to merging.

Activity

  1. added
    docsPull requests that change documentation only (docs: subject prefix)
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Sep 23, 2026
  2. JarryShaw commented on Sep 24, 2026

    @JarryShaw
    OwnerAuthor

    Recording the blocker, since this issue carried blocked with no reason written down anywhere — which makes the label unauditable. It is held on two things, both concrete:

    1. PR docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657 — the 1.5.0 changelog, still open. This sweep would edit docs prose; docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657 is 1,100+ lines of docs prose under active revision (eighteen review rounds so far). Touching the same surface means either a conflict or two people rewording the same sentences.
    2. Ordering, deliberately. A concision-and-accuracy sweep is only worth doing once the prose has stopped moving. refactor(protocols): import ProtocolBase under its own name in the last 54 sites #752 and feat(reg)!: split AppType into per-transport registries, dropping the portless rows #754 have just landed and both changed things the docs describe — ProtocolBase naming across 54 sites, and AppType becoming a package — so a sweep run today would be re-swept next week.

    What clears it: #657 merging. At that point the docs prose is stable and this becomes straightforwardly dispatchable.

    Worth noting one input it will want: #657's review established a repeatable method for exactly this kind of sweep — extract every cited repo path by four forms (slash path, dotted module, bare filename, elided), intersect with the diff since the merge base, and re-measure the intersection. A naive tense-keyword grep misses claims phrased as "48 of the 49", which is how that PR lost several rounds.

  3. JarryShaw commented on Sep 24, 2026

    @JarryShaw
    OwnerAuthor

    This one is deliberately blocked until all issues and PRs have been cleared.

  4. JarryShaw commented on Sep 24, 2026

    @JarryShaw
    OwnerAuthor

    Correcting my note above: I recorded the blocker as "#657 merging", which is too narrow. The maintainer's ground rule is broader — deliberately blocked until every issue and PR is cleared, the same posture #657 holds within the 1.5.0 cycle.

    So this is the last item on the board, not the next one after #657. Restating it so nobody reads my earlier line and dispatches this once #657 lands:

    The method note from my earlier comment still stands and is worth keeping: extract every cited repo path by four forms (slash path, dotted module, bare filename, elided), intersect with the diff since the merge base, and re-measure the intersection. #657's review established it after a tense-keyword grep missed claims phrased as "48 of the 49" — and that review then found the four-form method itself incomplete, needing ellipsis-elision and sibling elision too, plus a reverse sweep over the tracked file list to prove completeness. Six forms and a reverse check, not four.

  5. JarryShaw commented on Sep 25, 2026

    @JarryShaw
    OwnerAuthor

    Restating the blocker as the last comment, since this one is held by policy rather than by a dependency and that is easy to misread as neglect.

    Checkable blocker: the open-issue board is empty and every open PR sits at review: good-to-go.

    gh issue list -R JarryShaw/PyPCAPKit --state open --json number -q length
    gh pr list -R JarryShaw/PyPCAPKit --state open --json number,labels \
      -q '[.[]|select(([.labels[].name]|index("review: good-to-go"))==null)]|length'
    

    Both must read 0. This is a prose-concision sweep across the repo, so anything merged while it is in flight either escapes the sweep or forces a re-run over text that has since changed. Running it last is what makes one pass sufficient.

    Same reasoning holds #657, and the two are the deliberate last pair. Current board: 9 open issues, 2 open PRs.

  6. JarryShaw commented on Sep 26, 2026

    @JarryShaw
    OwnerAuthor

    One note on docs file formats, except those in docs/ directory's Sphinx docs must be using reST, other docs file should all be using markdown if applicable (meaning all READMEs should be markdown).

  7. JarryShaw commented on Sep 26, 2026

    @JarryShaw
    OwnerAuthor

    Recorded, and it corrects a standing convention I had been passing to every agent as "docs are .rst, never .md". Your words:

    "except those in docs/ directory's Sphinx docs must be using reST, other docs file should all be using markdown if applicable (meaning all READMEs should be markdown)."

    Measured the current state — the ruling's scope is exactly two files, and everything else already complies:

    .rst outside docs/    examples/benchmark/README.rst
                          examples/legacy_smoke/README.rst
    .md inside docs/      none
    already Markdown      README.md, CHANGELOG.md, CONTRIBUTING.md, CODE_OF_CONDUCT.md,
                          SECURITY.md, .github/ templates
    

    It is not a rename, though — MANIFEST.in makes it a packaging change. That file has global-include *.rst and prune docs, and there is no global-include *.md; the only .md lines are explicit include for the root README.md, CHANGELOG.md and CITATION.cff. So both example READMEs currently reach the sdist through global-include *.rst, and renaming them to .md silently drops them from every source distribution. CHANGELOG.md:75 records exactly this lesson from the root README's own conversion — "global-include *.rst no longer matches it".

    So whoever picks this up needs three changes, not one: rename both files, add matching coverage to MANIFEST.in, and update the live reference at examples/benchmark/benchmark.py:25, which points at :file:`README.rst` under "Departures from the legacy methodology" — the benchmark suite's own README, not the project's. Verify by building an sdist and diffing tar tzf before and after, which is how the MANIFEST.in comments say their claims were established.

    Checkable blocker: every other PR and issue closed.

    gh pr list -R JarryShaw/PyPCAPKit --state open --json number
    gh issue list -R JarryShaw/PyPCAPKit --state open --json number
    

    Unchanged: this issue and #657 merge last by your standing instruction, after the board is otherwise clear. Currently open besides these two: PR #836, and issues #775, #807, #808, #816.

  8. JarryShaw commented on Sep 27, 2026

    @JarryShaw
    OwnerAuthor

    Checkable blocker, recorded so this label is not taken on trust. #719 is blocked on the standing ordering
    rule: it and #657 land after every other PR and issue is closed. Two commands settle it at any moment:

    gh pr list -R JarryShaw/PyPCAPKit --state open --json number -q '.[].number' | grep -vE '^(657|719)$'
    gh issue list -R JarryShaw/PyPCAPKit --state open --json number -q '.[].number' | grep -v '^719$'

    Both must print nothing. As of now they do not — the outstanding set is:

    PRs    #856 (ci, review: good-to-go, CI running)    #855 (review: good-to-go, 58 ok / 0 inc)
           #657 (docs, merges last alongside this one)
    issues #842 covered by #855      #775 covered by #855 (tier 2 of it)
           #816 blocked on #775
    

    So the real gate is #775's remaining tiers, since #816 waits on it and #842 is part of it. Nothing here waits on a
    decision, and there is no work to start on this issue until that set empties.

    One thing already settled and worth keeping attached to this issue, since it changes what the sweep may touch: the
    docs convention is not "always .rst". Your ruling stands as recorded in the comment above this one —
    reStructuredText inside docs/, Markdown elsewhere. Any prose sweep run against this issue has to honour that
    split rather than converting files on sight.

  9. JarryShaw commented on Sep 29, 2026

    @JarryShaw
    OwnerAuthor

    on sphinx docs:

    1. changelog.rst should probably have a short summary of each change then link to their full changelog per version.
    2. testing/conventions/releasing.rst and other similar docs might go to another subdirectory to keep the toplevel docs clean.
    3. we did not have a pcapkit.__version__ doc entry and those :file:xxx references need to double check if using the right paths and making sure the rendered links are clickable.
  10. JarryShaw commented on Sep 29, 2026

    @JarryShaw
    OwnerAuthor

    Asks recorded, verbatim:

    on sphinx docs:

    1. changelog.rst should probably have a short summary of each change then link to their full changelog per version.
    2. testing/conventions/releasing.rst and other similar docs might go to another subdirectory to keep the toplevel docs clean.
    3. we did not have a pcapkit.__version__ doc entry and those :file:xxx references need to double check if using the right paths and making sure the rendered links are clickable.

    All three verified as real. The current top level is eight pages — changelog, conventions, demo, ext, index, pep, releasing, testing — against only two content subdirectories (changelog/, pcapkit/). And (3) is confirmed: every __version__ hit in the docs is prose in pep.rst:890 or releasing.rst:14,22,28,44; there is no API entry for it anywhere.

    These are structural, not prose, so they do not belong in this issue. #719 is the prose sweep and merges last under the standing ordering rule, which would gate three concrete pieces of work behind it for no reason. Filed separately — #899 (changelog summaries), #900 (subdirectory reorganisation), #901 (__version__ entry plus the :file: reference audit).

    One interaction worth flagging, and I have ruled on it rather than leaving it ambiguous. PR #898 went up minutes before your comment and adds docs/source/workflows.rst — a ninth top-level page, which is the growth (2) is complaining about. I am letting it land there anyway: moving one page into a new subdirectory while releasing.rst, testing.rst, conventions.rst and the rest stay put would create exactly the inconsistency you want removed. #900 moves all of them in one change, and #898's reviewer has been told not to treat the placement as a defect — only to flag it if the page carries anything a move would break.

    Say the word if you would rather #898 wait for #900 and land in the new location directly; that is the one alternative and it costs #898 a rebase rather than any rework.

  11. JarryShaw commented on Sep 29, 2026

    @JarryShaw
    OwnerAuthor

    Correction to my own comment above: the issue numbers I cited are wrong. I wrote them before filing and #899 was taken in between by a PR, so everything shifted by one. The real numbers are:

    There is no #899 in this set — that number belongs to the NGAP PR for #880. Everything else in the comment above stands, including the ruling that #898 lands at the top level and moves with the rest under #901.

  12. JarryShaw commented on Sep 29, 2026

    @JarryShaw
    OwnerAuthor

    amd one more thing, in the docs format, titles should have capitalised first letters of each word if applicable - i belie this is actually a house convention on documentations (Sphinx/docstrings).

  13. JarryShaw commented on Sep 29, 2026

    @JarryShaw
    OwnerAuthor

    Recorded, verbatim:

    amd one more thing, in the docs format, titles should have capitalised first letters of each word if applicable - i belie this is actually a house convention on documentations (Sphinx/docstrings).

    You are right that it is the de facto convention, and I measured how far from it the tree actually is. Counting every section title in the top-level pages (the line above each === / --- / ~~~ underline):

    total titles                62
    already Title Case          45
    carry a lowercase word      17
    

    So 73% follows it. And the biggest single violator is conventions.rst, with 8 of the 17 — most of which is my own writing from today and earlier:

    When an unrecognised value may mint a member
    The criterion
    Worked examples
    Why the company names mint
    Checking the current state
    Naming a sentinel
    What to implement, and what not to
    Where the registry protocol lives
    

    The rest are scattered — index.rst has "Engine support by Python version", "Engine prerequisites", "Indices and tables"; demo.rst and pep.rst account for a few more.

    Keeping this one in #719 rather than splitting it out, unlike your other three. Title casing is prose formatting and that is precisely this issue's charter, whereas #900/#901/#902 were structural and did not belong here.

    One sequencing note. #901 moves several of these pages into a subdirectory and #900 restructures changelog.rst, both of which touch headings. So the title sweep runs after those land, or it gets partly redone. I have told the #877 phase-1 agent to write any new heading it adds in Title Case so the same sweep does not have to revisit it, while leaving existing headings alone — those are yours here.

    Your "if applicable" is doing real work in that sentence, and I will treat it as excluding: headings that are a literal identifier (Why a class and not ``object()```), and short function words inside a title — standard Title Case leaves a, an, the, and, or, of, in, for, to` lowercase unless they lead. Say if you want strict every-word capitalisation instead; that changes several of the 45 that currently pass.

  14. JarryShaw commented on Sep 29, 2026

    @JarryShaw
    OwnerAuthor

    One thing about sentinel types and objects in the library: we should ONLY export the objects (like NULL) to __all__, and leave the types (like NullType) out.

    Also considering if we should move all these sentinels to a consolidated module (with multiple sub modules) or a dedicated single file.

  15. 246 remaining items

  16. JarryShaw commented on Oct 6, 2026

    @JarryShaw
    OwnerAuthor

    Round two is complete. All ten slices have merged:

    Slice PR
    data #1080
    internet #1081
    application #1082
    schema #1083
    foundation #1084
    rest of docs #1085
    docs/source/pcapkit/protocols #1086
    link/misc/transport/root #1087
    changelog #1088
    corekit and friends #1089

    48cdb589e adds one follow-up, a docs-only fix in ext.rst: Protocol is the user-facing base, and the built-in families derive from ProtocolBase.

    Each PR was cross-reviewed on a different model. Every pcapkit/ change was checked to leave the AST identical once docstrings are stripped.

    The slices also found code defects, but the sweep was prose-only, so none of them were fixed here. They will be filed as separate issues.

  17. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    breakingBreaks public-facing behaviour or API (apply alongside the type label)docsPull requests that change documentation only (docs: subject prefix)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions