Skip to content

Conversation

@Renegade334
Copy link
Member

@Renegade334 Renegade334 commented Oct 13, 2025

As implemented, .db and .capacity are getters, and .size is a callable method. There's no reason for this inconsistency, so change .size to a getter.

Drive-by changes while making alterations to the class template: the FunctionCallbackInfo parameter for SQLTagStore member functions was variably labelled args or info, and wasn't consistent with the header either. Use the canonical args.

@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 Oct 13, 2025
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 all of the stylistic changes should really be made separately.

@Renegade334 Renegade334 force-pushed the sqlite-tagstore-size-getter branch from dabca78 to 3142319 Compare October 13, 2025 15:23
@Renegade334
Copy link
Member Author

Renegade334 commented Oct 13, 2025

LGTM, but all of the stylistic changes should really be made separately.

I'm already half-addressing them de facto with the main change, but these can be spun out if desired.

Edit: Spun out the other doc changes.

@codecov
Copy link

codecov bot commented Oct 13, 2025

Codecov Report

❌ Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.56%. Comparing base (fbef1cf) to head (a217a01).
⚠️ Report is 133 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 80.95% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60246      +/-   ##
==========================================
- Coverage   88.58%   88.56%   -0.02%     
==========================================
  Files         704      704              
  Lines      207858   207857       -1     
  Branches    40054    40056       +2     
==========================================
- Hits       184131   184090      -41     
- Misses      15760    15806      +46     
+ Partials     7967     7961       -6     
Files with missing lines Coverage Δ
src/node_sqlite.h 80.39% <ø> (ø)
src/node_sqlite.cc 79.92% <80.95%> (-0.01%) ⬇️

... and 28 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 Renegade334 force-pushed the sqlite-tagstore-size-getter branch 2 times, most recently from ff66727 to a2eaaa0 Compare October 13, 2025 20:18
Drive-by: make callback `args` parameter names consistent
@Renegade334 Renegade334 force-pushed the sqlite-tagstore-size-getter branch from a2eaaa0 to a217a01 Compare October 31, 2025 13:53
@Renegade334
Copy link
Member Author

Rebased for #60474.

@Renegade334 Renegade334 added 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 Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

5 participants