Skip to content

sqlite,src: refactor sqlite value conversion - #57571

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
geeksilva97:sqlite-refactor-conversion-type
Apr 3, 2025
Merged

sqlite,src: refactor sqlite value conversion#57571
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
geeksilva97:sqlite-refactor-conversion-type

Conversation

@geeksilva97

@geeksilva97geeksilva97 commented Mar 21, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR is a minor refactor on converting SQLite values to JS. It makes the work for #56600 easier and reduces duplication.

@nodejs-github-bot

nodejs-github-bot commented Mar 21, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-botnodejs-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 Mar 21, 2025
@codecov

codecovBot commented Mar 21, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (1de917b) to head (94276bc).
Report is 74 commits behind head on main.

Files with missing linesPatch %Lines
src/node_sqlite.cc87.50%0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #57571 +/- ##
==========================================
- Coverage 90.23% 90.23% -0.01% 
==========================================
Files 629 630 +1 Lines 184903 185009 +106 Branches 36223 36221 -2 ==========================================
+ Hits 166853 166944 +91 - Misses 11010 11038 +28 + Partials 7040 7027 -13
Files with missing linesCoverage Δ
src/node_sqlite.cc78.20% <87.50%> (-0.29%)⬇️

... and 66 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.

@geeksilva97geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2025
@nodejs-github-bot

nodejs-github-bot commented Mar 21, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65862/

@Renegade334

Renegade334 commented Mar 21, 2025

Copy link
Copy Markdown
Member

Is this "returning function body macro" paradigm used elsewhere? Appreciate the rationale, but it seems like it lends potential for future mishaps?

@geeksilva97

geeksilva97 commented Mar 21, 2025

Copy link
Copy Markdown
ContributorAuthor

but it seems like it lends potential for future mishaps?

For this, specifically, I don't think so. This type conversion is unlikely to change. We need to find a way to isolate this anyways since it's being required more and more.

@geeksilva97geeksilva97 mentioned this pull request Mar 22, 2025
Merged
@geeksilva97geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2025
cjihrig
cjihrig reviewed Mar 22, 2025
View reviewed changes
Comment threadsrc/node_sqlite.cc Outdated
Comment threadsrc/node_sqlite.cc Outdated
Comment threadsrc/node_sqlite.cc Outdated
Comment threadsrc/node_sqlite.cc Outdated
Comment threadsrc/node_sqlite.cc Outdated
@geeksilva97
geeksilva97force-pushed the sqlite-refactor-conversion-type branch from 6cf644e to 2611aa7CompareMarch 24, 2025 20:44
@geeksilva97geeksilva97 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 25, 2025
cjihrig
cjihrig approved these changes Mar 26, 2025
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with one minor comment.

Comment threadsrc/node_sqlite.cc Outdated
@geeksilva97geeksilva97 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 26, 2025
@github-actionsgithub-actionsBot added the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 1, 2025
@github-actions

github-actionsBot commented Apr 1, 2025

Copy link
Copy Markdown
Contributor
Failed to start CI
 ⚠ Commits were pushed since the last approving review:
⚠ - sqlite,src: refactor sqlite value conversion
⚠ - fixup: apply review suggestions
⚠ - sqlite,src: improve conversion macro
✘ Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14196415493

@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 2, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 2, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65962/

@nodejs-github-bot

nodejs-github-bot commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/66000/

@cjihrigcjihrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Apr 3, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 8360ce2 into nodejs:mainApr 3, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator

Landed in 8360ce2

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@geeksilva97@RafaelGSS
sqlite,src: refactor sqlite value conversion
c123d5e
PR-URL: #57571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@geeksilva97@RafaelGSS
sqlite,src: refactor sqlite value conversion
da281d7
PR-URL: #57571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@geeksilva97@aduh95
sqlite,src: refactor sqlite value conversion
eae8119
PR-URL: #57571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@geeksilva97@RafaelGSS
sqlite,src: refactor sqlite value conversion
334441d
PR-URL: #57571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@geeksilva97@aduh95
sqlite,src: refactor sqlite value conversion
51f0fa9
PR-URL: #57571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@geeksilva97@aduh95
sqlite,src: refactor sqlite value conversion
11b0f0d
PR-URL: #57571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
@geeksilva97@aduh95
sqlite,src: refactor sqlite value conversion
00fbe64
PR-URL: #57571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@github-actionsgithub-actionsBot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@geeksilva97@aduh95
sqlite,src: refactor sqlite value conversion
120050d
PR-URL: #57571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ghostghost mentioned this pull request Jun 8, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@cjihrigcjihrigcjihrig approved these changes
@aduh95aduh95aduh95 approved these changes
@anonriganonrigAwaiting requested review from anonrig
@RafaelGSSRafaelGSSAwaiting requested review from RafaelGSS

Assignees

@geeksilva97geeksilva97

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.sqliteIssues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@geeksilva97@nodejs-github-bot@Renegade334@jasnell@cjihrig@aduh95