FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite,src: refactor sqlite value conversion by geeksilva97 · Pull Request #57571 · nodejs/node · GitHub

/ node Public

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
ConversationCommits (3)ChecksFiles changed
Merged

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

Conversation

geeksilva97 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 commented Mar 21, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

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 Mar 21, 2025

codecov Bot 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 lines Patch % Lines
src/node_sqlite.cc 87.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 lines Coverage Δ
src/node_sqlite.cc 78.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.

geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2025

nodejs-github-bot commented Mar 21, 2025

Copy link
Copy Markdown
Collaborator

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

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 commented Mar 21, 2025

Copy link
Copy Markdown
Contributor Author

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.

geeksilva97 mentioned this pull request Mar 22, 2025
Merged
geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2025
cjihrig reviewed Mar 22, 2025
View reviewed changes
Comment thread src/node_sqlite.cc Outdated
Comment thread src/node_sqlite.cc Outdated
Comment thread src/node_sqlite.cc Outdated
Comment thread src/node_sqlite.cc Outdated
Comment thread src/node_sqlite.cc Outdated
geeksilva97 force-pushed the sqlite-refactor-conversion-type branch from 6cf644e to 2611aa7 Compare March 24, 2025 20:44
geeksilva97 requested review from cjihrig and jasnell March 25, 2025 18:05
geeksilva97 added author ready PRs that have at least one approval, no outstanding review comments, 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 approved these changes Mar 26, 2025
View reviewed changes

cjihrig 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with one minor comment.

Comment thread src/node_sqlite.cc Outdated
geeksilva97 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 26, 2025
github-actions Bot 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 Bot 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

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, 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-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 2, 2025

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 commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator

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

cjihrig 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-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2025
nodejs-github-bot merged commit 8360ce2 into nodejs:main Apr 3, 2025

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
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>
RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
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
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
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
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
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
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-actions Bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
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>
ghost mentioned this pull request Jun 8, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
aduh95 aduh95 approved these changes
anonrig Awaiting requested review from anonrig
RafaelGSS Awaiting requested review from RafaelGSS

Assignees

geeksilva97

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

6 participants

Back | FazBrowse Home | New Git URL