-
Notifications
You must be signed in to change notification settings - Fork 5k
Remove dead code from the streams bindings, node:http, bun_sys, lsquic_sys, and orphaned files #38213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dead code from the streams bindings, node:http, bun_sys, lsquic_sys, and orphaned files #38213
Changes from 10 commits
afd3a55
91784db
9f99e10
b95d607
f53cc50
e183b85
e602c82
2c11b9e
0be40e1
abe40b4
c1758fb
59bf369
72cac7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The PR description's Verification section still claims
test/internal/source-lints/dead-symbols-streams-http-misctools.test.ts"pins everything above; all 40 content checks and 16 deleted-file checks fail against main and pass here", but tip commit 72cac7f deletes that file (added earlier in this PR, edited in 59bf369 for the hawk.toml regex nit, then removed with no stated reason). Dropping the guard is likely correct per REVIEW.md ("Do not add tests to check dead code stays dead"), so the fix is just to update the Verification section — and optionally note why this sweep omits a guard where the siblingdead-symbols-*.test.tsfiles kept theirs.Extended reasoning...
What the issue is
The tip commit of this PR is:
That file was added in e183b85, edited in c1758fb and 59bf369 (the latter narrowed the
hawk.tomlregex from/bun_platform|darwin::Category::/to/darwin::Category::/in response to the now-resolved inline review on this hunk), and then deleted entirely in 72cac7f with a bare commit message and no explanation in the PR body. Because it was added and deleted within the same PR, the file does not appear in the net diff at all.However, the PR description's Verification section still says:
And the robobun evidence block ("fails on main / passes on PR") shows that test running at commit 59bf369 — one commit before it was deleted. So both the Verification bullet and the evidence block now describe verification that no longer exists at tip.
Step-by-step proof
git log -- test/internal/source-lints/dead-symbols-streams-http-misctools.test.tsshows: added e183b85 → edited c1758fb → edited 59bf369 → deleted 72cac7f (tip).ls test/internal/source-lints/at tip: the file is absent, whiledead-symbols-install-sql-bindings.test.tsanddead-symbols-pub-exports-sweep.test.ts(from earlier sweeps) remain.59bf369d3.Why the deletion itself is probably fine
REVIEW.md's Code style section says explicitly: "Do not add tests to check dead code stays dead." The guard test was doing exactly that (regex-matching source files to prevent reintroduction of removed symbols), so a maintainer deleting it is consistent with current review guidance. The two sibling
dead-symbols-*.test.tsfiles predate that guidance and are not necessarily precedent to follow. The dead-code removals themselves are already verified by the build succeeding,cargo checkon all targets, and the functional test suites listed in the Verification section — the guard test was belt-and-suspenders, not load-bearing.Why it's still worth a comment
bun_platform|half of the alternation") was addressed in 59bf369, but the entire file was then deleted in the very next commit — so the resolution is moot, and that's not obvious from the thread.Impact
None at runtime — this is PR hygiene only. Nothing in the merged code would misbehave. Per the rubric, PR-description mismatches default to
nit, and there is no indication the code (as opposed to the description) is wrong.Fix
Update the PR description's Verification section to drop the last bullet about
dead-symbols-streams-http-misctools.test.ts(and optionally the robobun evidence block, which is auto-generated and will refresh on the next push anyway). If the guard was dropped deliberately per the "do not add tests to check dead code stays dead" rule, a one-line note to that effect would explain the divergence from the two siblingdead-symbols-*.test.tsfiles that remain in the directory.