Skip to content

Conversation

@neko-para
Copy link
Contributor

No description provided.

@mhdawson
Copy link
Member

@neko-para thanks for the PR, could you add a test as well?

@mhdawsonmhdawson moved this from Need Triage to In Progress in Node-API Team ProjectMay 2, 2025
@neko-para
Copy link
ContributorAuthor

Actually i'm not sure where should i add the tests. Maybe i shall add an extra entry?

@neko-para
Copy link
ContributorAuthor

@neko-para thanks for the PR, could you add a test as well?

Tests added.

@mhdawson
Copy link
Member

@neko-para just to confirm its not possible to use the existing operator:

 /// Converts an L-value to a value. operator Value() const; 

instead of adding the AsValue method ?

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.79%. Comparing base (b347dbe) to head (740d16c).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #1655 +/- ## ========================================== + Coverage 65.76% 65.79% +0.03%  ========================================== Files 3 3 Lines 2144 2146 +2 Branches 715 715 ========================================== + Hits 1410 1412 +2  Misses 162 162 Partials 572 572 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

@neko-para
Copy link
ContributorAuthor

@neko-para just to confirm its not possible to use the existing operator:

 /// Converts an L-value to a value. operator Value() const; 

instead of adding the AsValue method ?

The cast operator should always work. Adding the method is generally only for convenience, as explicit calling cast operator is not as intuitive as calling method.

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawsonmhdawson merged commit 1e57a0a into nodejs:mainMay 9, 2025
54 checks passed
@github-project-automationgithub-project-automationbot moved this from In Progress to Done in Node-API Team ProjectMay 9, 2025
@github-actionsgithub-actionsbot mentioned this pull request May 9, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@neko-para@mhdawson@codecov-commenter