Skip to content

sqlite: add ReadOptionalString helper - #67042

Open
araujogui wants to merge 1 commit into
nodejs:mainfrom
araujogui:sqlite-read-optional-string
Open

sqlite: add ReadOptionalString helper#67042
araujogui wants to merge 1 commit into
nodejs:mainfrom
araujogui:sqlite-read-optional-string

Conversation

@araujogui

@araujogui araujogui commented Oct 27, 2026

Copy link
Copy Markdown
Member

Dedupes string option parsing for dbName, table, db, source and target. A missing or undefined option now means the default everywhere; createSession() used HasOwnProperty, so { table: undefined } threw there.

Also marks ReadOptionalBool and ReadOptionalString as [[nodiscard]].

@araujogui
sqlite: add ReadOptionalString helper
e92cd96 
Use it for the dbName, table, db, source and target options so they
all treat a missing or undefined value as the default. Also mark
ReadOptionalBool and ReadOptionalString as [[nodiscard]].

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Copilot AI balanced review requested due to automatic review settings October 27, 2026 01:16
@nodejs-github-bot

nodejs-github-bot commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Oct 27, 2026
Copilot AI reviewed Oct 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@codecov

codecov Bot commented Oct 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.65%. Comparing base (352e814) to head (e92cd96).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #67042      +/-   ##
==========================================
- Coverage   92.96%   90.65%   -2.32%     
==========================================
  Files         428      797     +369     
  Lines      195232   278222   +82990     
  Branches    30197    53503   +23306     
==========================================
+ Hits       181500   252223   +70723     
- Misses      13405    16383    +2978     
- Partials      327     9616    +9289
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.44% <90.00%> (ø)

... and 501 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@araujogui @nodejs-github-bot