Skip to content

Conversation

geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Aug 28, 2025

This PR refactors SatetmentSync::All and StatementSyncIterator::Next methods to reduce complexity.

This will make it a lot easier to move forward with the implementation of the async api for node:sqlite

@nodejs-github-bot
Copy link
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 Aug 28, 2025
@geeksilva97 geeksilva97 force-pushed the refactor-sqlite-value-conversion branch from a7b35e6 to c64a47e Compare August 28, 2025 03:54
@geeksilva97 geeksilva97 marked this pull request as ready for review August 28, 2025 03:54
@geeksilva97 geeksilva97 force-pushed the refactor-sqlite-value-conversion branch from c64a47e to 45b12c8 Compare August 28, 2025 04:13
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

❌ Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.87%. Comparing base (bcb802c) to head (ae60602).
⚠️ Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 86.95% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59659      +/-   ##
==========================================
- Coverage   89.91%   89.87%   -0.04%     
==========================================
  Files         667      667              
  Lines      196600   196777     +177     
  Branches    38601    38622      +21     
==========================================
+ Hits       176780   176863      +83     
- Misses      12269    12361      +92     
- Partials     7551     7553       +2     
Files with missing lines Coverage Δ
src/node_sqlite.h 83.67% <ø> (ø)
src/node_sqlite.cc 81.28% <86.95%> (+0.17%) ⬆️

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

@Renegade334
Copy link
Contributor

Renegade334 commented Aug 31, 2025

Looking good. Is it worth doing the same with the column keys extraction? Obviously it wouldn't deduplicate anything now, but presumably would do if an async API were added?

@geeksilva97 geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2025
@nodejs-github-bot
Copy link
Collaborator

@geeksilva97 geeksilva97 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. labels Aug 31, 2025
@geeksilva97
Copy link
Contributor Author

Looking good. Is it worth doing the same with the column keys extraction? Obviously it wouldn't deduplicate anything now, but presumably would do if an async API were added?

Your assumption is correct. I can address this in the async api PR

@geeksilva97 geeksilva97 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Aug 31, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 31, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/59659
✔  Done loading data for nodejs/node/pull/59659
----------------------------------- PR info ------------------------------------
Title      src,sqlite: refactor value conversion (#59659)
Author     Edy Silva <[email protected]> (@geeksilva97)
Branch     geeksilva97:refactor-sqlite-value-conversion -> nodejs:main
Labels     c++, author ready, commit-queue-squash, sqlite
Commits    2
 - src,sqlite: refactor value conversion
 - src: use Maybe instead of std::optional
Committers 1
 - Edy Silva <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/59659
Reviewed-By: Anna Henningsen <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/59659
Reviewed-By: Anna Henningsen <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 28 Aug 2025 03:53:32 GMT
   ✔  Approvals: 1
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/59659#pullrequestreview-3171578413
   ✘  This PR needs to wait 83 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-08-31T14:06:05Z: https://ci.nodejs.org/job/node-test-pull-request/68973/
- Querying data for job/node-test-pull-request/68973/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/17359749713

@geeksilva97 geeksilva97 requested a review from himself65 August 31, 2025 21:35
@geeksilva97 geeksilva97 removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 31, 2025
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. 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
Development

Successfully merging this pull request may close these issues.

4 participants