-
-
Notifications
You must be signed in to change notification settings - Fork 400
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 boa_interop crate #3772
Add a boa_interop crate #3772
Conversation
This crate will contain types and functions to help integrating boa in Rust projects, making it easier to interop between the host and the JavaScript code. See boa-dev#3770
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3772 +/- ##
==========================================
+ Coverage 47.24% 49.71% +2.46%
==========================================
Files 476 456 -20
Lines 46892 44781 -2111
==========================================
+ Hits 22154 22261 +107
+ Misses 24738 22520 -2218 ☔ View full report in Codecov by Sentry. |
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.
Nice work on this! Thanks for presenting it and taking it on!
Below is some initial feedback after a quick overview.
impl HashMapModuleLoader { | ||
/// Creates an empty `HashMapModuleLoader`. | ||
#[must_use] | ||
pub fn new() -> Self { |
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.
suggestion/issue: not sure that new is needed here if all it does is call Default
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.
Some people don't like to import default (it's not part of std prelude). I've seen this pattern in places, including the rust std: https://github.com/rust-lang/rust/blob/master/library/std/src/collections/hash/map.rs#L229-L234
I'm okay replacing it with empty()
too if you want.
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.
I would say to just leave it. The std uses the same pattern of having a new
method that does the same as default
, so I think we should do the same.
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.
Good work!
impl HashMapModuleLoader { | ||
/// Creates an empty `HashMapModuleLoader`. | ||
#[must_use] | ||
pub fn new() -> Self { |
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.
I would say to just leave it. The std uses the same pattern of having a new
method that does the same as default
, so I think we should do the same.
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.
Nice work on this!
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.
Great work! Looks good to me! :)
This crate will contain types and functions to help integrating boa in Rust projects, making it easier to interop between the host and the JavaScript code.
See #3770