Skip to content

Conversation

@Renegade334
Copy link
Member

@Renegade334 Renegade334 commented Oct 10, 2025

Cleaning up a few issues:

  • The documentation talks about "interpolating" values into the string literal. The whole point of these functions is that they don't just perform string interpolation; they prepare a query string with placeholders and then bind the values passed into the template string.
  • The template string parameter binding behaviour was generally not really explained.
  • The argument types for the tagged query methods were incorrect.
  • .reset() was never implemented, only .clear(). There's no need for both to exist; the easiest thing is just to remove the dead reference.
  • The class definition block for StatementSync was sliced in half, messing with the doc tooling.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. sqlite Issues and PRs related to the SQLite subsystem. labels Oct 10, 2025
@Renegade334 Renegade334 force-pushed the doc-sqlite-tagstore branch 2 times, most recently from e9601ee to ca594f6 Compare October 10, 2025 14:40
@Renegade334 Renegade334 requested a review from cjihrig October 10, 2025 17:20
@Renegade334
Copy link
Member Author

Rebased for merge conflict.

@Renegade334 Renegade334 requested review from cjihrig and lpinca October 13, 2025 13:07
parameters to the underlying prepared statement. For example:

```js
tagStore.get`SELECT ${value}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tagStore should be defined, otherwise it is unclear how it is created.

Also I would recommend naming it sql or something else that is concise, since that is the whole point of the tagged literal API right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything it should be sqlTagStore to match the class block. I don't think the meaning is ambiguous, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding a

const tagStore = db.createTagStore();

at the top would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turtle problem, no? db isn't explicitly defined either!

This is really only intended to be an inline comment to show the difference between the two styles, and the full example block below still contains the concrete self-contained demonstration. I don't think there's much benefit to bloating these.

Copy link
Contributor

@louwers louwers Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other examples that use db as a name for a DatabaseSync handle.

This is really only intended to be an inline comment to show the difference between the two styles, and the full example block below still contains the concrete self-contained demonstration.

Fair point, maybe they can be swapped? First show the general usage, and then the difference?

I still think naming it sql would make the elegance of the API show, but I trust your judgement.

@Renegade334
Copy link
Member Author

Rebased for #60474.

@Renegade334 Renegade334 requested a review from cjihrig November 15, 2025 21:33
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@Renegade334 Renegade334 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 16, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 16, 2025
@nodejs-github-bot nodejs-github-bot merged commit 1ce22dd into nodejs:main Nov 16, 2025
31 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 1ce22dd

@Renegade334 Renegade334 deleted the doc-sqlite-tagstore branch November 16, 2025 01:20
targos pushed a commit that referenced this pull request Nov 27, 2025
PR-URL: #60200
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants