Skip to content

docs: four sites credit SQLBaseProvider with placeholder generation it does not have #647

Description

@cevheri

#640 counted five sites and I wrote the grep that counted them. It looked for the literal getPlaceholder, so it missed every site that paraphrases the name instead of writing it. #642 fixed the five and left four behind.

SQLBaseProvider has no placeholder logic of any kind. Measured on main at db40452, src/lib/db/providers/sql/sql-base.ts declares eight members and not one of them mentions a placeholder:

escapeIdentifier  buildLimitClause  shouldEnableSSL  getInformationSchemaName
getDefaultSchema  isReadOnlyQuery   isSchemaModifyingQuery  prepareQuery

The real home is positionalPlaceholder() in src/lib/sql/values.ts:148, a module-level function that no provider calls at all. Its only caller is src/hooks/use-inline-editing.ts:107.

The four sites

Site What it claims
docs/ADDING_A_PROVIDER.md:37 lists "placeholder style" among what SQLBaseProvider gives a new SQL provider
docs/DATABASE_PROVIDERS.md:108 "SQLBaseProvider provides SQL-specific helpers (LIMIT injection, identifier escaping, placeholder generation)"
docs/DATABASE_PROVIDERS.md:119 "identifier escaping and the placeholder style are inherited rather than rewritten"
docs/providers/postgres.md:42 "the shared SQL mechanics (identifier quoting, LIMIT injection, dialect placeholders, SSL auto-detection) live in SQLBaseProvider"

Two of these files already carry the correct version of the same fact, a couple of hundred lines away, so each one currently asserts both things:

  • docs/ADDING_A_PROVIDER.md:262 says positionalPlaceholder() is in values.ts and "not inherited" - while sitting under the heading What SQLBaseProvider adds.
  • docs/providers/postgres.md:83 says "shared rather than inherited".

The rule for the fix

Same one you applied in #642: keep the fact, drop the wrong attribution.

DATABASE_PROVIDERS.md:119 is the one to be careful with. It is a compound claim and only half of it is wrong: escapeIdentifier() really is inherited by ClickHouseProvider, DruidProvider and TrinoProvider (none of the three overrides it, ClickHouse only calls it), and double-quoted identifiers really are correct in all three dialects. Split the sentence rather than deleting it.

The wording docs/providers/clickhouse.md:126 and docs/providers/druid.md:131 now carry, after your PR, is the model:

Not in the list: a placeholder helper. SQLBaseProvider no longer has one (#304 removed it).

Two smaller things in the same sentence at docs/ADDING_A_PROVIDER.md:35-37, worth doing in the same edit:

The guard

This is the part that matters, and it is why this is a separate issue rather than a second commit on #642. Nothing in the suite mentions placeholders, so all nine sites went stale in silence and the four left could come back the same way. Add both of these to tests/unit/provider-docs-monitoring-citations.test.ts:

1. Extend the existing list. docs/DATABASE_PROVIDERS.md is not in TOP_LEVEL_NAMED_CITATION_DOCS (line 382). It carries zero .ts:<line> citations today, so adding it costs nothing and locks in what is already true.

2. A new test for the attribution. Assert that no line in these docs names SQLBaseProvider, or claims inheritance, together with a placeholder phrase.

Two traps to design around, both of which have bitten this file before:

  • Do not assert on the word. "docs/ADDING_A_PROVIDER.md must not contain placeholder" is a false-red machine: line 262 legitimately names positionalPlaceholder(), line 619 documents a searchPlaceholder label, and postgres.md:83 and :705 are both correct as written. Assert on the attribution, not the token.
  • Pair the negative with a control. A test that only asserts absence passes just as well when the docs are empty. Add a positive assertion of the fact it depends on: positionalPlaceholder is declared in src/lib/sql/values.ts, and sql-base.ts declares no member matching /placeholder/i. If someone later moves that logic into the base class, the control goes red and the guard has to be rewritten on purpose instead of quietly meaning nothing.

Verifying

  • grep -rniE 'placeholder' docs/ | grep -iE 'sqlbaseprovider|inherit' should return only sentences that are true.
  • bun run test stays green. postgres.md is in this file's NAMED_CITATIONS and ADDING_A_PROVIDER.md is in TOP_LEVEL_NAMED_CITATION_DOCS, so both are already in scope for the no-line-number tests.
  • The new test must fail on today's main. Write it first, watch it name the four sites, then fix them. A guard that was never seen red is not a guard.
  • 100% line coverage is a hard gate, but a test-only change adds no executable lines to src/.

@XiaoZ-0218 said on #642 they would take this. It is theirs.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentationgood first issueGood for newcomershacktoberfestCurated for Hacktoberfest; pick up cold, tests required

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions