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

src: remove node namespace from sqlite.cc #54500

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

src: remove node namespace from sqlite.cc #54500

wants to merge 3 commits into from
+45 −34

Conversation

JonasBa
Copy link
Contributor

@JonasBa JonasBa commented Aug 22, 2024

Remove unnecessary use of node namespace from node_sqlite.cc

@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 22, 2024
anonrig
anonrig reviewed Aug 22, 2024
View reviewed changes
src/node_sqlite.h Outdated Show resolved Hide resolved
@anonrig anonrig 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 Aug 22, 2024
cjihrig
cjihrig reviewed Aug 22, 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.

The string_view and Local<Value> changes should be done in separate commits since they aren't related to removing the namespace.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@JonasBa JonasBa force-pushed the jb/sqlite/node-namespace branch 3 times, most recently from 5f2f4a1 to 3d1ff26 Compare August 22, 2024 15:36
@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Aug 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@codecov Codecov
Copy link

codecov bot commented Aug 22, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.78788% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.03%. Comparing base (291d90a) to head (a773f11).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 78.78% 4 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54500      +/-   ##
==========================================
- Coverage   88.04%   88.03%   -0.01%     
==========================================
  Files         652      652              
  Lines      183764   183770       +6     
  Branches    35862    35863       +1     
==========================================
- Hits       161787   161781       -6     
+ Misses      15233    15232       -1     
- Partials     6744     6757      +13
Files with missing lines Coverage Δ
src/node_sqlite.h 0.00% <ø> (ø)
src/node_sqlite.cc 82.44% <78.78%> (-0.61%) ⬇️

... and 36 files with indirect coverage changes

jasnell
jasnell reviewed Aug 22, 2024
View reviewed changes
src/node_sqlite.cc Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Aug 22, 2024
View reviewed changes
src/node_sqlite.cc Outdated Show resolved Hide resolved
jasnell
jasnell approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits but otherwise LGTM

@anonrig
Copy link
Member

anonrig commented Aug 22, 2024

@JonasBa can you run the cpp formatter?

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
Copy link
Member

jasnell commented Sep 8, 2024

PR is currently blocked from landing due to unreliable CI. Likely needs a rebase

@lpinca
Copy link
Member

lpinca commented Sep 14, 2024

This needs a rebase to resolve conflicts.

@JonasBa
src: remove node namespace from sqlite.cc
5b4d2e1 
src: remove unnecessary node namespace

src: remove string view changes
@JonasBa
src: use string view instead of local
4ded9ab 
This reverts commit 1340a81744b5a934b4ea0423bc01d424fa152127.

src: use direct initialzation and const string view signature
@JonasBa
fixup! src: remove node namespace from sqlite.cc
a773f11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig left review comments

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig approved these changes

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++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. 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
@JonasBa @nodejs-github-bot @anonrig @jasnell @lpinca @cjihrig