Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqlite: allow returning ArrayBufferViews from user-defined functions #56790

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Renegade334
Copy link
Contributor

#56385 widened the valid "blob-equivalent" type from Uint8Array to ArrayBufferView for bound parameters. This isn't currently reflected in the valid return types for user-defined functions, and it seems sensible to keep things consistent.

@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 Jan 27, 2025
@Renegade334 Renegade334 marked this pull request as ready for review January 27, 2025 15:48
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.21%. Comparing base (752c0b8) to head (29ff781).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56790   +/-   ##
=======================================
  Coverage   89.21%   89.21%           
=======================================
  Files         663      663           
  Lines      192012   192012           
  Branches    36929    36931    +2     
=======================================
+ Hits       171296   171300    +4     
- Misses      13577    13580    +3     
+ Partials     7139     7132    -7     
Files with missing lines Coverage Δ
src/node_sqlite.cc 79.65% <100.00%> (ø)

... and 25 files with indirect coverage changes

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 29, 2025
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 29, 2025

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

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 29, 2025
@cjihrig cjihrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 30, 2025
@cjihrig
Copy link
Contributor

cjihrig commented Jan 30, 2025

It looks like this should be able to land but the commit queue is not picking it up - possibly because GitHub thinks there are still two pending checks?

@Renegade334
Copy link
Contributor Author

The runs are complete on Jenkins, I guess GHA must have been having a moment...

@Renegade334 Renegade334 force-pushed the sqlite-function-arraybufferview branch from 9fb613e to 29ff781 Compare January 31, 2025 01:12
@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 Jan 31, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/56790
✔  Done loading data for nodejs/node/pull/56790
----------------------------------- PR info ------------------------------------
Title      sqlite: allow returning `ArrayBufferView`s from user-defined functions (#56790)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     Renegade334:sqlite-function-arraybufferview -> nodejs:main
Labels     c++, semver-minor, author ready, needs-ci, sqlite
Commits    1
 - sqlite: allow returning `ArrayBufferView`s from user-defined functions
Committers 1
 - Renegade334 <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56790
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56790
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - sqlite: allow returning `ArrayBufferView`s from user-defined functions
   ℹ  This PR was created on Mon, 27 Jan 2025 15:30:10 GMT
   ✔  Approvals: 3
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/56790#pullrequestreview-2580127244
   ✔  - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/56790#pullrequestreview-2581083910
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56790#pullrequestreview-2581360993
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-01-29T20:29:00Z: https://ci.nodejs.org/job/node-test-pull-request/64832/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - sqlite: allow returning `ArrayBufferView`s from user-defined functions
- Querying data for job/node-test-pull-request/64832/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/13065470909

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-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants