Skip to content

/ rust-mozjs

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

Rewrite FromJSValConvertible to better support unions #284

Merged
merged 1 commit into from Aug 19, 2016
Merged

Rewrite FromJSValConvertible to better support unions #284

merged 1 commit into from Aug 19, 2016

Conversation

@GuillaumeGomez
Copy link
Contributor

GuillaumeGomez commented Aug 5, 2016 •
edited

Fixes #282.

This change is Reviewable
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 5, 2016

r? @nox
@Ms2ger Ms2ger self-assigned this Aug 6, 2016
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 6, 2016

Updated.
src/conversions.rs Outdated
if value.get().is_null_or_undefined() {
Ok(None)
Ok(ConversionResult::Failure(Cow::Owned("Value is null or undefined.".to_owned())))

This comment has been minimized.

Sign in to view
@emilio

emilio Aug 6, 2016

Member

What's the point of returning a Cow if we only return owned values? Can't this be Cow::Borrowed("Value is null or undefined")?

This comment has been minimized.

Sign in to view
@GuillaumeGomez

GuillaumeGomez Aug 6, 2016

Author Contributor

cc @nox

This comment has been minimized.

Sign in to view
@emilio

emilio Aug 8, 2016

Member

@GuillaumeGomez: I'm pretty sure being able to return a borrowed cow is the reason for @nox to propose it, can you change it in the relevant places?

This comment has been minimized.

Sign in to view
@Ms2ger

Ms2ger Aug 8, 2016

Collaborator

This is not a failure case; should just stay Ok(None).
src/conversions.rs Outdated
} else {
let result: Result<T, ()> = FromJSValConvertible::from_jsval(cx, value, option);
result.map(Some)
match FromJSValConvertible::from_jsval(cx, value, option) {

This comment has been minimized.

Sign in to view
@Ms2ger

Ms2ger Aug 8, 2016

Collaborator

You can use try!() here
src/conversions.rs Outdated
@@ -553,7 +576,7 @@ impl<C: Clone, T: FromJSValConvertible<Config=C>> FromJSValConvertible for Vec<T
let iterator = &mut *iterator.root;

if !iterator.init(value, ForOfIterator_NonIterableBehavior::ThrowOnNonIterable) {
return Err(())
return Ok(ConversionResult::Failure(Cow::Owned("Not iterable".to_owned())))

This comment has been minimized.

Sign in to view
@Ms2ger

Ms2ger Aug 8, 2016

Collaborator

This doesn't agree with the ThrowOnNonIterable argument passed above. Please keep returning Err(())
@Ms2ger
Copy link
Collaborator

Ms2ger commented Aug 8, 2016

convert_int_from_jsval and enforce_range need to be changed to return Result<ConversionResult<D/T>, ()>
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 8, 2016

Updated.
src/conversions.rs
@@ -129,7 +149,7 @@ unsafe fn enforce_range<D>(cx: *mut JSContext, d: f64) -> Result<D, ()>

let rounded = d.round();
if D::min_value().cast() <= rounded && rounded <= D::max_value().cast() {
Ok(rounded.cast())
Ok(ConversionResult::Success(rounded.cast()))
} else {
throw_type_error(cx, "value out of range in an EnforceRange argument");

This comment has been minimized.

Sign in to view
@GuillaumeGomez

GuillaumeGomez Aug 8, 2016

Author Contributor

Should I return Ok(ConversionResult::Failure) here?

This comment has been minimized.

Sign in to view
@Ms2ger

Ms2ger Aug 8, 2016

Collaborator

Yep.

This comment has been minimized.

Sign in to view
@Ms2ger

Ms2ger Aug 8, 2016

Collaborator

Actually, no, I don't think so.

This comment has been minimized.

Sign in to view
@GuillaumeGomez

GuillaumeGomez Aug 8, 2016

Author Contributor

Ok.
@Ms2ger
Copy link
Collaborator

Ms2ger commented Aug 19, 2016

@bors-servo r+
@Ms2ger Ms2ger closed this Aug 19, 2016
@Ms2ger Ms2ger reopened this Aug 19, 2016
@Ms2ger
Copy link
Collaborator

Ms2ger commented Aug 19, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 19, 2016

📌 Commit 9a01c9d has been approved by Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Aug 19, 2016

⌛ Testing commit 9a01c9d with merge cac3beb...
bors-servo added a commit that referenced this pull request Aug 19, 2016
@bors-servo
Auto merge of #284 - GuillaumeGomez:rewrite_from_js_val, r=Ms2ger
Loading status checks…
cac3beb 
Rewrite FromJSValConvertible to better support unions

Fixes #282.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/284)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 19, 2016

💔 Test failed - status-travis
@GuillaumeGomez
Rewrite FromJSValConvertible to better support unions
Loading status checks…
9b2e4ad
@Ms2ger
Copy link
Collaborator

Ms2ger commented Aug 19, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 19, 2016

📌 Commit 9b2e4ad has been approved by Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Aug 19, 2016

⌛ Testing commit 9b2e4ad with merge daffcfb...
bors-servo added a commit that referenced this pull request Aug 19, 2016
@bors-servo
Auto merge of #284 - GuillaumeGomez:rewrite_from_js_val, r=Ms2ger
Loading status checks…
daffcfb 
Rewrite FromJSValConvertible to better support unions

Fixes #282.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/284)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 19, 2016

☀️ Test successful - status-appveyor, status-travis
@bors-servo bors-servo merged commit 9b2e4ad into servo:master Aug 19, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:rewrite_from_js_val branch Aug 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@GuillaumeGomez @Ms2ger @bors-servo @emilio
You can’t perform that action at this time.