Skip to content
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

doc: clarify expandedSQL behavior #54685

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 1, 2024

I am not sure what exactly the primary use case is for this function given that the Node.js API is not designed for users to manually bind parameters of prepared statements, but this at least clarifies what the function does.

I am not sure what exactly the primary use case is for this function
given that the Node.js API is not designed for users to manually bind
parameters of prepared statements, but this at least clarifies what the
function does.
@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 Sep 1, 2024
@targos
Copy link
Member

targos commented Sep 1, 2024

Maybe the purpose is debugging?

@tniessen
Copy link
Member Author

tniessen commented Sep 1, 2024

Yeah, maybe users can then copy-paste the SQL query into a different SQLite interface for debugging. I don't think there's any pre-processing besides substituting named parameters for the values.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 1, 2024

Maybe the purpose is debugging?

Yes, that's it. There is value in being able to see what SQL a prepared statement evaluates to.

@tniessen
Copy link
Member Author

tniessen commented Sep 1, 2024

Ah, I assumed the SQLite developers' intention was to enable this without having to actually execute the statement, hence I was confused by the design here. But that makes sense, thank you!

@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Sep 1, 2024
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.

LGTM, but I think the text here will be inaccurate once #54721 lands.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 4, 2024

Specifically just the use of "method"

@tniessen tniessen added commit-queue Add this label to land a pull request using GitHub Actions. and removed review wanted PRs that need reviews. labels Sep 4, 2024
@tniessen
Copy link
Member Author

tniessen commented Sep 4, 2024

Thank you @cjihrig. I will rebase #54721 as necessary. I haven't started CI on that PR yet, and considering the state that our CI is in, I don't expect it to land anytime soon (even though I hope to be proven wrong).

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2024
@nodejs-github-bot nodejs-github-bot merged commit 6c6a933 into nodejs:main Sep 4, 2024
30 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 6c6a933

aduh95 pushed a commit that referenced this pull request Sep 12, 2024
I am not sure what exactly the primary use case is for this function
given that the Node.js API is not designed for users to manually bind
parameters of prepared statements, but this at least clarifies what the
function does.

PR-URL: #54685
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
I am not sure what exactly the primary use case is for this function
given that the Node.js API is not designed for users to manually bind
parameters of prepared statements, but this at least clarifies what the
function does.

PR-URL: nodejs#54685
Reviewed-By: Michaël Zasso <[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. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants