docs(clickhouse): cite code by name, not by line - #639
Conversation
clickhouse.md pinned 16 citations to line numbers in index.ts, transport.ts, introspect.ts and connection-string-parser.ts (the 17th, factory.ts:87, went with libredb#629), and 13 of them were stale: prepareQuery() cited :562 is declared at :596, resolveConnection() :402 at :420, getCapabilities() :461 at :503, getLabels() :489 at :553, query() :593 at :674, runMaintenance() :920 at :1013, and both index.ts:637 cites land on a closing brace today. Each now names the declaration and links the file the way mssql.md, mongodb.md and postgres.md already do; the two error-classification cites name mapClickHouseError(), the verbatim-type one names readColumn(). The doc joins NAMED_CITATIONS in the guard, so it can carry no `.ts:<line>` and every method it names must really be declared in index.ts. Reverting the doc alone fails 2 of 31. Fixes libredb#588 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cevheri
left a comment
There was a problem hiding this comment.
Thanks for this. Replacing line-number citations with named ones is exactly the right direction: line numbers go stale on the next edit to the provider, names do not, and the new guard entry turns the doc into something CI can keep honest. I verified all 16 rewritten citations resolve against the head commit, confirmed no .ts:NNN citations remain in the file, checked every relative link, and ran the guard test at PR head (31/31 pass). Formatting is clean.
Two things I would like updated before this lands. Neither is a correctness problem with what you wrote, they are both about the doc and the guard agreeing with each other.
1. getPlaceholder() in section 2.3 (line 123) does not exist any more.
That row is outside the diff, so I could not leave an inline note on it. It lists getPlaceholder() as an inherited SQLBaseProvider member, but the method was deleted in #304 (commit 02578ff, "had no callers") and is not present anywhere under src/. It is pre-existing, and the same stale name appears in mssql.md, oracle.md, druid.md, elasticsearch.md and opensearch.md, so fixing all of them is not your job here. But it is a name-based citation that fails to resolve, in the one file this PR rewrote to make name-based citations trustworthy, so it fits naturally in this change. The nuance the row was making (ClickHouse binds only named {name:Type} parameters, so a positional ? never applies) is worth keeping. It just needs to stop attributing that to a method that no longer exists. The other five docs can follow in a separate PR if you would like to take them.
2. The guard entry measures 6 methods, and the doc names about 20.
See the inline note. The PR body says "every method it names must really be declared", and that is the promise worth making. Right now the entry is narrower than the doc, which is why the getPlaceholder case above slips through unmeasured. Widening the list closes both findings at once.
Happy to take a second look as soon as you push. If you would rather split item 1 out, say so and I will file it separately, but the two really do belong together: item 2 is what makes item 1 impossible to reintroduce.
| { | ||
| doc: "docs/providers/clickhouse.md", | ||
| source: "src/lib/db/providers/sql/clickhouse/index.ts", | ||
| methods: ["mapClickHouseError", "prepareQuery", "query", "runMaintenance", "getCapabilities", "getLabels"], |
There was a problem hiding this comment.
The doc names roughly 20 symbols from index.ts (validate, connect, disconnect, getSchema, getHealth, getOverview, getSlowQueries, getActiveSessions, getPerformanceMetrics, getTableStats, getStorageStats, getIndexStats, and more), and every one of them is declared with an access modifier, so declarationLine() can match them exactly the way it matches the six listed here. Nothing is blocking the wider list.
That gap is what lets the stale getPlaceholder() row in section 2.3 sit unnoticed: the guard never looks at it. Could you extend methods to cover the symbols the doc actually cites? A short comment above the entry saying that the list is meant to track the doc, not a hand-picked subset, would help whoever adds the next provider entry keep the same bar.
Worth knowing for the follow-up: getPlaceholder() lives on the base class rather than in index.ts, so a widened list here will not catch inherited-member rows on its own. Naming only what index.ts declares is the correct scope for this entry. Item 1 in my summary is the manual fix for the inherited-member table.
There was a problem hiding this comment.
Widened to all 20 in 4e1edc8: every name( the doc cites that index.ts declares with an access modifier, in declaration order, with a comment above the entry saying the list tracks the doc rather than a subset. I recomputed that intersection from the doc and the source with a throwaway script and compared it against the entry: 20/20, same order.
One cited index.ts name is deliberately not in it: resolveConnection() is a module-level function (index.ts:420), so declarationLine() has nothing to match. The comment names it so the next person does not add it and wonder why the test goes red.
31/31 pass, biome / oxlint / eslint clean on the file.
… cited method Section 2.3 listed getPlaceholder() as an inherited SQLBaseProvider member, but libredb#304 (02578ff) deleted it and nothing under src/ declares it. The row's point survives as a sentence under the table: ClickHouse binds named {name:Type} parameters only, so a positional `?` never applies and query() throws rather than send one unbound. The NAMED_CITATIONS entry named 6 of the 20 index.ts methods the doc cites. It now names all 20, in declaration order, with a comment saying the list tracks the doc rather than a hand-picked subset. Module-level functions (resolveConnection) and inherited members carry no access modifier for declarationLine() to match, so they stay unmeasured, as the PR body says. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Both in 4e1edc8.
|
|
Merging this. The update closes both points, and I measured the claims rather than taking them:
Two follow-ups, both filed and neither required for this PR:
Thanks for the write-up on both rounds. The intersection rule you put in the comment is the part that makes this hard to reintroduce. Edited: the third bullet originally cited |
Description
docs/providers/clickhouse.mdcited code by line number in 16 places (the 17th in #588,factory.ts:87, already went with #629), and on today'smain13 of them are stale — the four that name a method beside the line all miss, and bothindex.ts:637cites now land on a closing brace:prepareQuery()index.ts:562:596prepareQuery()resolveConnection()index.ts:402:420resolveConnection()getCapabilities()index.ts:461:503getCapabilities()getLabels()index.ts:489:553getLabels()query()index.ts:593:674query()runMaintenance()index.ts:920:1013runMaintenance()index.ts:637(×2, error classification by exception code)}mapClickHouseError()RAW_TEXT_COLUMNindex.ts:102:105RAW_TEXT_COLUMNtransport.ts:114(ClickHouseTransport):127ClickHouseTransporttransport.ts:178(ClickHouseTransportError):191ClickHouseTransportErrorintrospect.ts:290(declared type kept verbatim)readColumn()at:327readColumn()connection-string-parser.ts:66parseConnectionString()at:96parseConnectionString()transport.ts:37,transport.ts:69,introspect.ts:186ClickHouseQueryResult,ClickHouseQueryResult.mutationCount,isNullableType()Every citation now names the declaration and links the file without a coordinate, the shape
mssql.md,mongodb.mdandpostgres.md(#636) already have.grep -oE '\.ts:[0-9]+' docs/providers/clickhouse.mdreturns nothing.The doc joins
NAMED_CITATIONSwithsrc/lib/db/providers/sql/clickhouse/index.tsas its source and the twenty class methods it cites by name — everyname(the doc cites thatindex.tsdeclares with an access modifier, in declaration order — so the guard's two tests now assert the doc carries no.ts:<line>and that each of those is really declared. The names intransport.ts,introspect.tsand the parser, andresolveConnection()inindex.tsitself, are module-level declarations the guard'sdeclarationLinedoes not read, so they are cited but not measured — the same standing aspostgres.md'ssql-base.tsnames.Type of Change
Related Issue
Fixes #588
Changes Made
docs/providers/clickhouse.md: 16 citations rewritten; §2.3'sgetPlaceholder()row (the method went with fix(sql): bind the inline row edit's values, and quote the remaining interpolated ones (#290) #304) is replaced by a sentence under the table that keeps the named-parameters-only pointtests/unit/provider-docs-monitoring-citations.test.ts: oneNAMED_CITATIONSentryTesting
bun test tests/unit/provider-docs-monitoring-citations.test.tsmaincites no line number anywhere,names methods that … really declares)biome check,oxlint,eslinton the test file andtsc --noEmitare clean. Run under Bun 1.3.11 (the repo pins 1.4.2); this test only reads files.