-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Add a try_from_js implementation for Vec<T> (accept any Array-like) #3755
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I just have a small suggestion, but everything else looks good!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3755 +/- ##
==========================================
+ Coverage 47.24% 47.80% +0.56%
==========================================
Files 476 454 -22
Lines 46892 44594 -2298
==========================================
- Hits 22154 21319 -835
+ Misses 24738 23275 -1463 ☔ View full report in Codecov by Sentry. |
The only concern here would be on how to implement this for TypedArrays, which should have a much faster conversion. Not sure if we would need to specialize or we could just add another branch. |
I suggest a specialized type, like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! 😄 Had one nit, but that's not blocking merging.
@@ -250,3 +283,42 @@ impl TryFromJs for u128 { | |||
} | |||
} | |||
} | |||
|
|||
#[test] | |||
fn value_into_vec() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you add a test or two for a failing cases as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. What could I do to unblock this? It's blocking some of my follow work that I'd like to submit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you for the contribution!
This PR adds an implementation of
TryFromJs
forVec<T: TryFromJs>
to create Rust vectors from Array-like JavaScript objects.This is useful when using typed arguments to native functions with arrays.