Fix the WebSocket doc examples so they typecheck - #58
Conversation
WebSocket.send takes a &String — encode-text hands msg to String.to-bytes — but every documented example passed an owned String: @"connected" for the literal and a bare (fmt ...) for the formatted case. Every documented way to send on a WebSocket was code that cannot compile, and the doc strings are also the generated API reference, so each error shipped twice. Sites fixed: the web.carp file-header prelude, the App.WS and App.WSP doc strings, the README WebSocket section, and the gendocs.carp docs prelude. Each was reproduced as a failing scratch build first and rebuilt clean after the fix. docs/ regenerated with gendocs.carp. docs/index.html is a hand-kept copy of the generated web_index.html (byte-identical before this change) and was resynced. send-binary takes an &(Array Byte) and was already correct.
There was a problem hiding this comment.
Build & Tests
Checked out 89cafcc. Merge-base is 285a3ab = current origin/main, so the changelog-conflict question does not arise and no entry is mis-filed.
- The three fixed examples compile and run. I pulled all of them — the
web.carpprelude /App.WS/READMEhandler, thegendocs.carpvariant without theBinaryarm, and theApp.WSPchathandler — into one scratch file, registered them on anApp, and ran it: rc 0, prints and exits clean. - The old form genuinely fails, checked rather than assumed:
I can't match the types `String` and `(Ref String e)`. within `(copy "connected")` "connected" : (Ref String t14) Expected first argument to 'copy' : (Ref (Ref String e) r13) docs/regenerates to exactly what is committed. I rancarp -x gendocs.carpon this branch andgit status docs/came back empty — no drift, no stale file, and nothing hand-edited into the generated HTML.- CI
test (macos-latest)— pass, run'shead_shaconfirmed as89cafcc.
Findings
1. A sixth site was missed: docs/websocket-design.md:20-21
The body says "Two substitutions, applied at every site" and enumerates five. There is a sixth, carrying the identical broken pair:
docs/websocket-design.md:20: (WSEvent.Connect) (WebSocket.send ws @"connected")
docs/websocket-design.md:21: (WSEvent.Message msg) (WebSocket.send ws (fmt "echo: %s" msg))
That is byte-for-byte the code I compiled above to produce the (Ref String e) error, so it is the same defect, not a lookalike.
I checked the grep has teeth before reporting an absence — the same pattern run against origin/main finds every site the PR fixed (README, both web.carp doc strings, gendocs.carp, and all three generated HTML files) plus this one; run against 89cafcc it finds this one and nothing else. So the sweep is complete in both directions: this is the last remaining instance in the repo, and it is the only one.
Two honest caveats, because they may change what you want done:
- It is a design record —
# WebSocket Design (issue #11), introduced byf22b25a "initial websocket work"and never touched since. It is not linked from the README, not linked fromdocs/index.html, and not generated bygendocs.carp. - But its section heading is "User API (as implemented)", and it is the first thing under
docs/that a reader browsing the directory hits, so it reads as current documentation rather than as an archived proposal.
If you consider a design record frozen, then merging as-is is right and the fix is to the PR body instead — drop "every site", since the claim as written is not true. If you consider it documentation, it is the same two substitutions applied one more time.
2. Checked and clean
docs/index.htmlreally is a hand-kept copy, exactly as the body claims, and I verified it independently rather than taking it: it is byte-identical todocs/web_index.htmlonmainand on this branch,save-docsonly writes<title>_index.html(Project.config "title"is"web"), and the two files have moved together in all eight commits that touched either. Resyncing it was correct, not churn.- The untouched sites are untouched for the right reason.
WebSocket.send-binarytakes&(Array Byte)and thedatafrommatch-refis already a reference;bench/ws_echo.carp:5passesmsgstraight through frommatch-ref, so it is already a&String. Both compile as-is. - No other doc example has this bug. I checked the remaining
@"..."arguments inweb.carp's doc strings against their signatures:Response.textstoresbodyowned,Response.chunkedtakeschunksby reference and the example passes&[...], andApp.WSP'sprotocolsis an owned(Array String). All three are correct as documented. - No CHANGELOG entry is the right call — this changes documentation, not behaviour, and the changelog carries no docs-only entries in its history.
Verdict: revise
The fix itself is correct, minimal and properly verified — examples built rather than reasoned about, docs regenerated faithfully, and the index.html claim holds up under independent checking. It needs one more pass only because the PR's own standard is exhaustiveness and the sweep stopped one file short: docs/websocket-design.md still tells readers to write the code that does not compile. Two lines, or one sentence out of the PR body if you would rather leave the design record alone — but not both left as they are.
The sweep stopped one file short: docs/websocket-design.md carried the identical broken pair under its "User API (as implemented)" heading, so docs/ still told readers to write code that does not compile. Same two substitutions as the other five sites: @"..." -> "..." and (fmt ...) -> &(fmt ...), since WebSocket.send feeds its argument to encode-text, which does (String.to-bytes msg) and so needs a &String. Verified by extracting the snippet into a scratch file registered on an App: the old form fails with `I can't match the types String and (Ref String e)` within `(copy "connected")`, the new form compiles and runs clean under an otherwise byte-identical harness.
|
@carpentry-reviewer — finding 1 is right, and it's documentation, not a frozen record: the heading is “User API (as implemented)”, so I fixed the file rather than weakening the claim. I built it rather than reasoning about it. Two scratch files whose diff is exactly those two lines, each registering the The grep, teeth-checked in the direction you'd want. I widened it to be quoting-agnostic first, because my initial pattern silently missed
I also added the sixth row to the PR body's table and the build/grep evidence to its Verification section, so the “every site” claim now matches what the diff actually does. |
There was a problem hiding this comment.
Build & Tests
Checked out 851fa5c. Merge-base with main is 285a3ab, which is current origin/main, so nothing is mis-filed against a release commit.
-
CI
test (macos-latest)— pass, and I confirmed the run'shead_shais851fa5crather than the previous round's commit. -
carp -x test/websocket.carp— 132 passed, 0 failed, rc 0. -
docs/regenerates to exactly what is committed.carp -x gendocs.carpexits 0 and leavesgit status --porcelainempty. -
The fixed snippet compiles as written. I did not check only the two changed lines: I pulled the whole
docs/websocket-design.mdblock — thechathandler and the(defserver ... (GET ...) (WS ...))form it is registered by — into a scratch file and built it. Two harnesses differing by exactly those two lines:; old form ; new form I can't match the types `String` and rc=0 `(Ref String e)`. within `(copy "connected")` "connected" : (Ref String t14) Expected first argument to 'copy' : (Ref (Ref String e) r13) rc=1
Prior feedback
Finding 1 — the sixth site — is addressed. docs/websocket-design.md:20-21 now carries the same two substitutions as the other five, and the build above is against the design record's own snippet rather than a lookalike. You also took the second half of the ask and made the PR body match: the table is six rows now, so "applied at every site" is a true claim about this diff.
Finding 2 was a list of things I had checked and found clean; nothing there needed action, and nothing in 851fa5c disturbs it — docs/index.html and docs/web_index.html are still byte-identical.
Findings
None. I re-ran the exhaustiveness check in both directions rather than trusting the previous round's:
- The quoting-agnostic pattern (
WebSocket.send <ident> @andWebSocket.send <ident> (fmt, covering@"...", the doc-string@\"...\"and the HTML@"...") finds 20 lines across 8 files onorigin/mainand 0 on851fa5c. It has teeth in the direction that matters. - I also enumerated every
WebSocket.send/send-binarycall site at851fa5cinstead of relying on one pattern, and each survivor is correct for its own reason:bench/ws_echo.carp:5andtest/websocket.carp:127,162were already passing references (msgout ofmatch-ref, and a bare string literal tosend-now), and the threesend-binarylines take&(Array Byte), which thedatafrommatch-refalready is.
The three [WARNING] ... changed type of 'Response.not-found' / 'bad-request' / 'redirect' lines that show up in every build here are pre-existing on main and untouched by this PR.
Verdict: merge
The one issue from the previous round is fixed at exactly the lines it was raised against, verified by building the design record's snippet rather than reasoning about it, and the sweep now checks out as exhaustive from both ends. Tests pass, docs are in sync, and CI is green on this head.
WebSocket.sendisand
encode-textdoes(String.to-bytes msg), somsgis a&String. Everydocumented example passed an owned
Stringinstead —@"connected"for theliteral and a bare
(fmt ...)for the formatted case — so every documented wayto send on a WebSocket was code that cannot compile. The doc strings are also
the generated API reference, so each error was published twice.
What changed
Two substitutions, applied at every site:
@"..."→"..."and(fmt ...)→&(fmt ...). No prose reworded, no example restructured.web.carpfile-header preludeWebmodule doc stringApp.WSdoc stringdocs/App.htmlApp.WSPdoc stringdocs/App.html— not previously flaggedREADME.mdWebSocket sectiongendocs.carpdocs preludedocs/web_index.htmldocs/websocket-design.md“User API (as implemented)”docs/docs/was regenerated withcarp -x gendocs.carp.save-docsonly writes<title>_index.html, sodocs/index.htmlis a hand-kept copy ofweb_index.html— byte-identical to it onmain— and was resynced so thepublished landing page carries the fix too.
WebSocket.send-binarytakes an&(Array Byte); those lines were alreadycorrect and are untouched.
bench/ws_echo.carppassesmsgstraight throughfrom
match-ref, so it was already a&Stringand is untouched.Verification
Each example was extracted into a scratch file and built, not reasoned
about — failing first, clean after:
After the fix a single scratch file holding all three handlers (
echo,chat,clock) registered on anAppcompiles and runs clean.The
docs/websocket-design.mdsnippet was built the same way, under a harnessthat differs only in those two lines: the old form fails with
I can't match the types String and (Ref String e)within(copy "connected"), the new formcompiles and prints clean. A form-agnostic grep for the two broken argument
shapes —
WebSocket.send ws @andWebSocket.send ws (fmt— finds 20 linesacross 8 files on
origin/mainand 0 on this branch, so it has teeth in thedirection that matters.
carp -x test/websocket.carp— 132 passed, 0 failed.angleroutput onweb.carpandgendocs.carpis byte-identical tomain's (the remainingfindings — uppercase
GET/WS/WSPnames, two nested-if chains — arepre-existing).
carp-fmt -cfails on both files onmainas well, so it wasleft alone rather than reformatting the whole file.
No CHANGELOG entry: this fixes documentation, it does not change behaviour.
Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.