Skip to content

fix(vendor): make wrapped const comments render as written (#1846, #1847) - #1871

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1846-vendor-comment-rest
Oct 17, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1846-vendor-comment-rest

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

  • Vendor.wrap_comment puts a space after a URL reference that another
    bracket follows, which docutils autolinked into the next citation, and
    backslash-escapes a leading reST enumerator such as (S).
  • Vendor.wrap_comment and AppType.tabulate pass break_on_hyphens=False: a
    continued line joins with a space, so a hyphen split rendered "run- time"
    and cut a list-table cell's URL short.
  • AppType.wrap_comment takes its lines from the base instead of wrapping on
    its own, so it gets both fixes; nothing it emits trips the fix(vendor): generated comments run a URL reference into the next bracket and start a line with (S), which reST misrenders #1846 cases yet.
  • Re-render the affected const comments and AppType tables offline from the
    committed text, with no crawl. Members, values and code are unchanged.
  • Add tests/vendor/test_vendor_wrap_comment_unit.py.

Closes #1846
Closes #1847

The const files were re-rendered offline from their committed text, so no crawler ran. Each is AST-identical to main with docstrings blanked, and a member dump of every changed module is byte-identical.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) const Regenerated IANA or vendor constant tables; members keep their numeric values review: running A cross-review is in flight against the current head - no verdict yet labels Oct 16, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at db1129257. A Sonnet cross-review (author: Opus) confirmed the following:

  • Enumerator escape. Only (S) flag in mh/dhcp_support_mode.py matches the escape rule. Look-alikes such as X.509, AX.25, H.248 and e.g. are untouched, and docutils agrees on 35 probes.
  • Hyphen rejoins. Every rejoined word is a single word upstream, checked against the IANA service-names, SCTP, HTTP and protocol-numbers registries and tcpdump's linktypes page.
  • Wrapper output. The patched wrappers reproduce every wrapper-made #: block and all the AppType list-tables byte for byte, so the next crawl will not re-diff them.
  • Members unchanged. A dump of every enum member in every pcapkit.const module is byte-identical to main.
  • Tests. The generator tests fail on main's templates.

Non-blocking:

  • the enumerator check looks for a literal space, so a newline or tab after (S) would slip through;
  • AppType table cells do not get the URL spacing.
    Neither occurs in the data today.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at db1129257. This replaces the earlier GOOD TO GO. CI's Python 3.14 job fails 18 tests in tests/test_tier_guard.py (job 117260332315), all with the same message: 'docutils' has no MODULE_PROVIDERS entry.

The cause is the new HAS_DOCUTILS gate in tests/vendor/test_vendor_wrap_comment_unit.py. The dependency-gate guard needs every gated module mapped to the distribution and extra that provide it, or, if no extra carries it, an exclusion saying why. The cross-review never ran tests/test_tier_guard.py, so it missed this. A worker is on the fix.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 17, 2026
@JarryShaw
JarryShaw force-pushed the fix/1846-vendor-comment-rest branch from db11292 to b20fe2f Compare October 17, 2026 02:07
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at b20fe2fef. The CI fix is correct: tests/test_tier_guard.py went from 18 failed on db1129257 to 111 passed. A Sonnet cross-review also confirmed the HAS_DOCUTILS exclusion's dark set matches what the guard derives, exactly the test and gate jobs.

One required change, a false comment. tests/_dependency_gates.py (~239) says listing the bare docutils string lets a future extra that declares it close the gap without editing this entry. Measured: with docutils added to an extra, docutils becomes a contested import, and test_mutually_exclusive_imports_matches_its_derivation fails until the entry is cut to the bare name. So reword it to say that, or drop the sentence, and keep both tuple entries. In the same edit, reword "one distribution reached two ways" so it carries no count.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 17, 2026
@JarryShaw
JarryShaw force-pushed the fix/1846-vendor-comment-rest branch from b20fe2f to b671834 Compare October 17, 2026 02:49
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at b671834b7. The reworded MODULE_PROVIDERS['docutils'] comment is now true, and the reviewer re-ran the experiment to check it:

  • with docutils declared in the test extra, the entry must be cut to the bare name and the exclusion deleted;
  • with docutils declared in the docs extra, only the entry must change.

test_tier_guard.py gives 111 passed.

One change left, the same wording error I've flagged on #1877 and #1881. The new HAS_DOCUTILS reason says it needs "the docs extra that only deploy-pages.yml installs". No workflow installs the docs extra. deploy-pages.yml:167-168 installs docs/requirements.txt (which brings Sphinx, and with it docutils) and .[all,dev]. Reword it to say that.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
)

- Vendor.wrap_comment puts a space after a URL reference that another
  bracket follows, which docutils autolinked into the next citation, and
  backslash-escapes a leading reST enumerator such as (S).
- Vendor.wrap_comment and AppType.tabulate pass break_on_hyphens=False: a
  continued line joins with a space, so a hyphen split rendered "run- time"
  and cut a list-table cell's URL short.
- AppType.wrap_comment takes its lines from the base instead of wrapping on
  its own, so it gets both fixes; nothing it emits trips the #1846 cases yet.
- Re-render the affected const comments and AppType tables offline from the
  committed text, with no crawl. Members, values and code are unchanged.
- Add tests/vendor/test_vendor_wrap_comment_unit.py.
- Map docutils in tests/_dependency_gates.py, via Sphinx, and record the
  HAS_DOCUTILS gate as deliberately dark on test and gate.

Closes #1846
Closes #1847
@JarryShaw
JarryShaw force-pushed the fix/1846-vendor-comment-rest branch from b671834 to adf8f9c Compare October 17, 2026 02:56
@JarryShaw JarryShaw removed the review: needs-changes Cross-review at the current head says changes are required; see the verdict comment label Oct 17, 2026
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at adf8f9c4f. The Sonnet reviewer checked the last change, which touches only the HAS_DOCUTILS reason. Against .github/workflows/, its claims hold:

  • no pytest job installs docutils;
  • no workflow installs the docs extra;
  • deploy-pages gets Sphinx, and with it docutils, from docs/requirements.txt.

The reason states no count, and tests/test_tier_guard.py gives 111 passed. The pre-existing HAS_SPHINX reason, which has the same error, is corrected in #1877.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 90.05% (unit tier, Python 3.15, adf8f9c4f, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 2 20 2 96.77%
pcapkit/const 19595 881 2618 867 92.11%
pcapkit/corekit 2462 69 780 35 96.55%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2914 164 1016 34 93.59%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17406 224 4580 189 98.10%
pcapkit/toolkit 872 177 298 8 77.18%
pcapkit/utilities 512 2 158 4 99.10%
pcapkit/vendor 4409 2342 1010 157 43.29%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 0d6601c into main Oct 17, 2026
57 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 17, 2026
@JarryShaw
JarryShaw deleted the fix/1846-vendor-comment-rest branch October 17, 2026 13:19
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

const Regenerated IANA or vendor constant tables; members keep their numeric values fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

1 participant