Repository navigation
README: render the callouts on GitHub, not just in Sphinx - #400
Merged
Merged
Conversation
GitHub renders reStructuredText through plain docutils, which knows the
`note`/`important` directives but emits them as a class-less `<div>` containing
the label as an ordinary paragraph. Fetched from the live page rather than
guessed at:
<div dir="auto">
<p dir="auto">Important</p>
<p dir="auto">The <code>all</code> extra deliberately does <strong>not</strong> ...
So all three admonitions showed up as the bare word "Note" or "Important"
stranded on its own line above the text, with no callout box and nothing marking
them as asides. Zero elements carrying an `admonition` class reached the page.
Rewritten as blockquotes with a bold lead, which GitHub renders with its left
bar. This is not a new convention for the file -- the README already used exactly
this shape in two other places, so the three directives were the odd ones out.
Nothing else in the README needs it: the eight `code-block:: shell` blocks all
highlight correctly (docutils 0.22 registers `code-block`), the `list-table`
becomes a real table, and all ten named hyperlink targets resolve. Verified by
counting the rendered elements, not by eye.
README.rst is not included in the Sphinx tree, so this trades nothing on the
docs side.
There was a problem hiding this comment.
🟡 Changes recommended
The new standalone .. lines start RST comment blocks, causing the intended callout text to be commented out and not rendered.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR aims to make the README render callout-style notes correctly on GitHub by replacing Sphinx-style admonition directives with GitHub-friendly markup.
Changes:
- Replaced
.. note::/.. important::admonitions with intended blockquote-style callouts using bold leads.
File summaries
| File | Description |
|---|---|
| README.rst | Converts admonition directives to GitHub-renderable callout formatting in the README |
Review details
Suppressed comments (2)
README.rst:212
- This standalone
..begins an RST comment block; the subsequent indented Important text becomes part of the comment and won’t render. Remove the..line so the indented paragraph is a visible blockquote callout.
..
README.rst:265
..on its own is an RST comment directive, which causes the following indented Note block to be commented out (not rendered). Remove the..line to keep the callout visible as a blockquote.
..
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: JarryShaw <15666417+JarryShaw@users.noreply.github.com>
JarryShaw
added a commit
that referenced
this pull request
Sep 16, 2026
… clean The benchmark table's footnotes were hand-numbered `[1]_`..`[4]_`, and referenced out of order -- the table cited 3, 1, 4, 2 -- so any insertion meant renumbering every marker and definition by hand. They are now RST auto-numbered footnotes with labels (`[#pyshark-historical]_` and friends), which is the form already used across `docs/source/pcapkit/const/*.rst` as auto-symbol `[*]_`. Labelled rather than bare `[#]_` on purpose: bare auto-numbers are assigned in reference order, so the definitions would have to stay in the table's order to read correctly, and a reordered row would silently repoint a marker at the wrong note. Labels make the pairing explicit and order-independent. Labelled auto-footnotes do take their *numbers* from definition order, though, so the definitions are also reordered to match the table -- otherwise the markers rendered 3, 1, 4, 2, exactly the wart being removed. Verified: markers now render 1, 2, 3, 4 in both the table and the definition list, in `README.rst` and `docs/source/index.rst`. Also removed three Sphinx-only roles this branch had introduced into `README.rst` -- one `:manpage:` and two `:func:` -- which `main` did not have. GitHub renders the README with plain docutils, which knows none of them and emits a visible `Unknown interpreted text role` error for each, the same defect #400 fixed for the admonitions. They are now ``literals``. README again renders with zero diagnostics under plain docutils.
JarryShaw
added a commit
that referenced
this pull request
Sep 16, 2026
…reflight (#405) * engines: add the pcap_ct engine, and make unsupported_reason a real preflight Upstream `pypcap` is unusable on a current Python: 1.3.0 compiles a `pcap.c` pre-generated by Cython 0.29.32, which does not build against the 3.12+ C API, and it publishes no wheels. Measured with libpcap present and found: builds on 3.10 and 3.11, fails on 3.12 (`ob_digit`, `curexc_traceback`) and 3.14 (those plus `ma_version_tag` and `_PyLong_AsByteArray` arity). `pcap-ct` is a ctypes reimplementation that provides the same top-level `pcap` module and works across the whole supported range. It is added as its own engine rather than a second backend behind `pypcap`, because the two are **mutually exclusive** -- both distributions own the import name `pcap`, and with both installed pcap-ct wins while upstream's extension module is shadowed and unreachable. One engine silently meaning two different implementations is exactly the ambiguity worth avoiding. New shared, stdlib-only `_pcap_backend.py` is the single place that answers "which backend did I get": `hasattr(pcap, '_pcap')` distinguishes them (pcap-ct is a package, upstream a lone extension module), and `importlib.metadata` reveals the distribution the import did *not* resolve to. Shared deliberately -- two copies would be two chances for the engines to disagree. Each engine exposes `.backend` and warns when both are installed. `unsupported_reason()` now works as a preflight across every engine, which is the case the import test cannot cover: a dependency that installs cleanly and only fails when used. * `pcap`, `pcapng` -- no override; pcapkit's own parsers have no requirement. * `dpkt`, `scapy` -- no override, measured genuinely unconstrained on 3.14. * `pyshark` -- ceiling 3.14 plus a tshark check. It *imports* on 3.14 and fails at use: the call it makes, `asyncio.get_event_loop_policy().get_event_loop()`, returns a loop on 3.10/3.11, warns on 3.12 and raises on 3.14. 3.13 was not available and is inferred, flagged as such in the code. * `pypcap`, `pcap_ct` -- wrong-backend detection; `pcap_ct` also reports a missing system libpcap. * `pypcapfile` -- unchanged from #396. The tshark check delegates to pyshark's own `get_process_path()` rather than `shutil.which`, which is not equivalent: pyshark consults `tshark_path` in its config first, then PATH on POSIX, both Program Files directories on Windows, and `/Applications/Wireshark.app` on macOS, so `which` would refuse working setups. Not cached -- the failing path measures ~287 us against 39 PATH entries and runs once per Extractor, and caching would freeze an answer about the *environment*, so installing Wireshark would not take effect until restart. Three of my own earlier claims that measurement disproved, corrected in the docs rather than left standing: the `libpcap` wheel does **not** remove the need for a system libpcap (its published config sets `LIBPCAP = None`, sending the loader to `find_library("pcap")`); with no libpcap, `import pcap` raises `OSError`, not `ImportError`, so it escapes `import_test` and aborts the extraction rather than degrading; and the PCAP-NG verdict is host-dependent -- libpcap 1.11 reads `dhcp.pcapng` correctly while 1.5.3 silently returns nonsense timestamps, so both engines keep rejecting PCAP-NG, now with the real reason. Fixed while probing: a second probe after a failure reported `NameError: '__about__' is not defined` instead of the real cause, because both `pcap-ct`'s and `libpcap`'s `__init__` open with a non-rerunnable `from .__about__ import * ; del __about__`. `probe()` now purges both module trees on failure. Also moved `test_new_engine_parity.py` to `*_runtime.py`. It read four generated captures from the unit tier -- a tier violation that #393's guard could not catch, because it calls `sample_path(capture)` with a *variable*, which only the runtime layer sees, and that layer is never reached on a host where the engine packages are absent. It was the sole cause of the 5 failures in the pristine baseline below. `examples/legacy_smoke/_engine_support.py` listed only four engines, so the timing harness never exercised the new ones; its skip messages now consult `unsupported_reason()` and name the real cause. No benchmark numbers are published. They were measured, but this host is a shared cloud desktop under load whose figures disagree with the README's existing row in *both* directions, so mixing the two tables row-for-row would be misleading. An engine x Python support matrix is documented instead, which is host-independent: 3.11 is the last version where every engine runs, and even there pypcap and pcap_ct are mutually exclusive, so no single environment has all seven. Verified after rebasing onto 89201df: engine identity asserted rather than inferred (`__engine_name__ == 'PCAP_CT'`, 6 frames, no EngineWarning); `unsupported_reason()` exercised for all eight engines; `files=True` naming checked against #403's bare-`_fext` change, which this engine's per-frame naming depends on. Unit tier 614 passed / 28 skipped / 0 failed, against a pristine-main baseline of 5 failed / 546 passed / 50 skipped. * docs: use auto-numbered footnotes, and keep the README plain-docutils clean The benchmark table's footnotes were hand-numbered `[1]_`..`[4]_`, and referenced out of order -- the table cited 3, 1, 4, 2 -- so any insertion meant renumbering every marker and definition by hand. They are now RST auto-numbered footnotes with labels (`[#pyshark-historical]_` and friends), which is the form already used across `docs/source/pcapkit/const/*.rst` as auto-symbol `[*]_`. Labelled rather than bare `[#]_` on purpose: bare auto-numbers are assigned in reference order, so the definitions would have to stay in the table's order to read correctly, and a reordered row would silently repoint a marker at the wrong note. Labels make the pairing explicit and order-independent. Labelled auto-footnotes do take their *numbers* from definition order, though, so the definitions are also reordered to match the table -- otherwise the markers rendered 3, 1, 4, 2, exactly the wart being removed. Verified: markers now render 1, 2, 3, 4 in both the table and the definition list, in `README.rst` and `docs/source/index.rst`. Also removed three Sphinx-only roles this branch had introduced into `README.rst` -- one `:manpage:` and two `:func:` -- which `main` did not have. GitHub renders the README with plain docutils, which knows none of them and emits a visible `Unknown interpreted text role` error for each, the same defect #400 fixed for the admonitions. They are now ``literals``. README again renders with zero diagnostics under plain docutils. * docs: auto-symbol footnotes in the Sphinx matrix, literal symbols in the README The engine-support matrix hand-wrote its `*` and `†` markers in the cells with loose prose beneath, so nothing linked and the symbols had to be maintained by hand. In `docs/source/index.rst` those are now RST auto-symbol footnotes (`[*]_`), which index themselves. `README.rst` deliberately keeps the literal `*`/`†` characters. GitHub renders it with plain docutils, which does not present indexed footnotes usefully, so the markers there stay plain text -- the same reasoning that keeps admonitions out of that file. One constraint worth recording, because it shaped the layout: **auto-symbol footnotes are strictly one reference per definition.** Two references against one definition is an error (`Too many symbol footnote references: only 1 corresponding footnote available`), and three references mint three *separate* notes rendering `*`, `†`, `‡`. Since `*` appeared in eight cells and `†` in four, a direct conversion would have needed twelve definitions. The markers therefore moved off the individual cells and onto the things they actually qualify -- the `3.13` and `3.15` column headers, and the `pyshark` row label -- which is three references, three definitions, and no loss of precision about which verdicts are inferred. The benchmark table's `[1]_`..`[4]_` footnotes are left numbered as they were. Verified: README renders with zero diagnostics under plain docutils; the Sphinx copy's three auto-symbol footnotes resolve without warnings.
JarryShaw
added
the
docs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You were right that the README leans on Sphinx-flavoured RST that GitHub does not fully render. I checked the live page rather than reasoning about it, and the problem is narrower than it looked — but real.
What actually breaks
GitHub renders RST through plain docutils, which does know
noteandimportant, but emits them as a class-less<div>with the label as an ordinary paragraph. Pulled from the rendered page:So all three admonitions appeared as the bare word "Note" or "Important" stranded on its own line above the text — no callout box, nothing marking them as an aside. Counting elements on the live page: 0 carrying an
admonitionclass, and 3 bare<p>Note</p>/<p>Important</p>paragraphs.Rewritten as blockquotes with a bold lead, which GitHub renders with its left bar. This is not a new convention for the file — the README already used exactly this shape in two other places, so the three directives were the odd ones out:
What does not need changing, verified rather than assumed
I audited the whole file against what GitHub's renderer supports, and the rest is fine:
.. code-block:: shellcode-block, and the page shows 9highlight-source-shellblocks.. list-table::<table>(5 tables on the page)dpkt.readthedocs.io,en.wikipedia.org/wiki/Pcap,libpcapand the rest are livehrefs:mod:,:class:,:envvar:, …)So no
code-block→codechurn, and no link rewriting. Only the three admonitions were actually broken.Rendering the result with plain docutils at
report_level=1gives zero diagnostics, 5 blockquotes, and 0 admonitions.README.rstis not included in the Sphinx tree — nothing underdocs/source/includes it — so this trades nothing on the docs side.