Skip to content

refactor(corekit)!: move chardet into corekit (#1712) - #1729

Closed
JarryShaw wants to merge 1 commit into
mainfrom
refactor/1712-chardet-into-corekit
Closed

JarryShaw wants to merge 1 commit into
mainfrom
refactor/1712-chardet-into-corekit

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

Description of your pull request and other information

Closes #1712. pcapkit.utilities.chardet moves to pcapkit/corekit/chardet.py, with no re-export left behind (the owner's ruling on #1715).

  • Breaking: importing pcapkit.utilities.chardet now raises ModuleNotFoundError. detect also leaves pcapkit.utilities and pcapkit.all.
  • Why directly under corekit: Protocol.decode calls it as well as StringField, so it has to sit below both. It imports only functools and chardet, so it cannot close a cycle.
  • Importers updated: strings.py (its import line only), protocol.py, ftp.py and httpv1.py, plus the tests and the all-exports guard.
  • Docs: the page moves to docs/source/pcapkit/corekit/chardet.rst. Sphinx gives 63 warnings, the same set as main, and objects.inv lists only the new module.
  • Tests:
    • tests/corekit/test_chardet_housing_1712_unit.py checks that the old path is gone and the new module stays a leaf. It fails on main and passes on 3.8 and 3.14.
    • The StringField, FTP/HTTP and public-API guard tests pass.

The bounded-cache chardet wrapper moves from pcapkit.utilities.chardet to
pcapkit.corekit.chardet, with no re-export at the old path (the ruling on
#1715, applied to #1712).

* detect and DETECT_CACHE_SIZE live in pcapkit/corekit/chardet.py, directly
  under corekit rather than in corekit.fields, since Protocol.decode calls it
  as well as StringField. It imports nothing from pcapkit, so no cycle.
* pcapkit.utilities drops detect from its imports and __all__, so
  pcapkit.utilities.detect and pcapkit.all.detect go too.
* strings.py (import line only), protocol.py, ftp.py and httpv1.py import
  from the new path; the docs page moves to the corekit toctree.

Tests: test_chardet_housing_1712_unit fails 6 of 7 on 4c750fe and passes
on 3.8 and 3.14; the export, public-API, StringField, FTP and HTTP/1 tests
pass, and Sphinx gives 63 warnings before and after.

Closes #1712
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 14, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 945638d63. A Sonnet cross-review checked it; Opus wrote it.

  • Old path gone: git grep and grep -r for the dotted, slash and backslash forms find the old path only in the new test. utilities.detect and all.detect are gone too.
  • No import cycle: each of 13 modules was imported first in its own fresh interpreter, on 3.8 and 3.14, and all 26 runs passed. The new module imports only functools and chardet, and every caller holds the same detect.
  • Guards and docs: the public-API and all-exports guards pass. Sphinx gives 63 warnings, the same set as main, and objects.inv lists only pcapkit.corekit.chardet.
  • Fail before: the negative test fails on main.
  • Breaking: the subject carries !.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Heads-up: this conflicts with #1730 (#1709) in pcapkit/protocols/protocol.py, where both PRs edit the import lines. Either can merge first, and I will rebase the other.

@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 89.35% (unit tier, Python 3.15, 945638d63, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 3 20 3 95.16%
pcapkit/const 18860 954 2368 868 91.06%
pcapkit/corekit 2379 67 762 35 96.50%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2885 144 1010 34 94.20%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17181 226 4590 190 98.07%
pcapkit/toolkit 1148 290 450 10 71.21%
pcapkit/utilities 500 4 154 4 98.78%
pcapkit/vendor 4417 2342 1010 157 43.38%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Holding: the owner is reconsidering where corekit ends and utilities begins (#1712). This waits for that criterion before merging.

@JarryShaw

JarryShaw commented Oct 14, 2026 •

Copy link
Copy Markdown
Owner Author

Closing unmerged. Under the rule the owner chose on #1712 (option A), chardet is a third-party wrapper service, so it stays in utilities. The rule's one move, decorators into corekit, is tracked in #1735.

@JarryShaw JarryShaw closed this Oct 14, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 14, 2026
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

refactor(corekit): move pcapkit.utilities.chardet into corekit

1 participant