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.

Sign up for GitHub

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

Jump to bottom

sqlite: improve error handling using MaybeLocal #55571

Merged
merged 1 commit into from
Nov 4, 2024
Merged

sqlite: improve error handling using MaybeLocal #55571

merged 1 commit into from
Nov 4, 2024

Conversation

tniessen
Copy link
Member

As per @jasnell's suggestion, consistently use MaybeLocal and avoid Check() and ToLocalChecked() entirely.

@tniessen
sqlite: improve error handling using MaybeLocal
e20695a 
As per James' suggestion, consistently use MaybeLocal and avoid Check()
and ToLocalChecked() entirely.

Refs: nodejs#54687
@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 28, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 28, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 64.70588% with 12 lines in your changes missing coverage. Please review.

Project coverage is 88.42%. Comparing base (5d4fee8) to head (e20695a).
Report is 65 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 64.70% 6 Missing and 6 partials ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #55571   +/-   ##
=======================================
  Coverage   88.42%   88.42%           
=======================================
  Files         654      654           
  Lines      187662   187681   +19     
  Branches    36118    36130   +12     
=======================================
+ Hits       165945   165962   +17     
+ Misses      14955    14952    -3     
- Partials     6762     6767    +5
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.83% <64.70%> (-1.53%) ⬇️

... and 26 files with indirect coverage changes

cjihrig
cjihrig approved these changes Oct 28, 2024
View reviewed changes
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.

FWIW, I think I copied this from somewhere else in the codebase, so there are probably other places where this change could be applied.

@tniessen tniessen 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. labels Oct 28, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@tniessen tniessen added experimental Issues and PRs related to experimental features. review wanted PRs that need reviews. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 29, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 4, 2024
@nodejs-github-bot nodejs-github-bot merged commit bdc2662 into nodejs:main Nov 4, 2024
84 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in bdc2662

aduh95 pushed a commit that referenced this pull request Nov 5, 2024
@tniessen @aduh95
sqlite: improve error handling using MaybeLocal
151bb16 
As per James' suggestion, consistently use MaybeLocal and avoid Check()
and ToLocalChecked() entirely.

Refs: #54687
PR-URL: #55571
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
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++. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. 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.
3 participants
@tniessen @nodejs-github-bot @cjihrig