Conversation
Test262 conformance changes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4603 +/- ##
==========================================
+ Coverage 47.24% 56.50% +9.25%
==========================================
Files 476 548 +72
Lines 46892 60090 +13198
==========================================
+ Hits 22154 33952 +11798
- Misses 24738 26138 +1400 ☔ 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().
core/runtime/src/process/mod.rs
Outdated
| /// | ||
| /// # 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.
There was a problem hiding this comment.
Yup I see. Will do it!
core/runtime/src/process/mod.rs
Outdated
| 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());
There was a problem hiding this comment.
Oh I see. I'll get rid of it right away!
5647a8c to
fcd7050
Compare
fcd7050 to
16067b3
Compare
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: