Conversation
Test262 conformance changes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4603 +/- ##
==========================================
+ Coverage 47.24% 56.54% +9.29%
==========================================
Files 476 548 +72
Lines 46892 60093 +13201
==========================================
+ Hits 22154 33977 +11823
- Misses 24738 26116 +1378 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jedel1043
left a comment
There was a problem hiding this comment.
Implementation wise looks great! I just have an API suggestion and a comment on the error thrown when calling current_dir().
| /// | ||
| /// # Errors | ||
| /// Returns an error if the environment variables cannot be obtained. | ||
| fn env(&self) -> JsResult<HashMap<String, String>>; |
There was a problem hiding this comment.
I would suggest just using impl IntoIterator<(JsString, JsString)> as the return value here, to mirror what std::env::vars() returns. IMO we really don't need a fallible method here.
| impl ProcessProvider for StdProcessProvider { | ||
| fn cwd(&self) -> JsResult<JsString> { | ||
| let path = std::env::current_dir() | ||
| .map_err(|e| JsError::from_opaque(js_string!(e.to_string()).into()))?; |
There was a problem hiding this comment.
It's not that useful to return an opaque error here; just use TypeError and pass e.to_string() as the message for the error, maybe with a nice error prefix to show that this error comes from calling std::env::current_dir()
There was a problem hiding this comment.
By the way you can easily format nice error messages with our js_error macro:
js_error!(TypeError: "nice error message. source: {error}", e.to_string());
It changes the following:
Uses a provider pattern so as to allow custom
ProcessProviderfor embedded systems where using the defaultStdProcessProviderwill not be proper.Adds
process.cwd()so as to allow: