Skip to content
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

Improve Boolean/Number/JsString consistency #1447

Merged
merged 1 commit into from
Apr 12, 2019

Conversation

alexcrichton
Copy link
Contributor

  • Ensure PartialEq is implemented from these types to native Rust types
  • Implement From between these type and native Rust types
  • Deprecated Number::new and Boolean::new to discourage use of the
    object forms, recommending the from constructors instead.

Closes #1446

crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
}
)*)
}
number_from!(i8 u8 i16 u16 i32 u32 f32 f64);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a silly suggestion, but why not use transitive generics instead of a custom macro? Something like:

impl<T> From<T> for Number where f64: From<T> {
  // ...
}

The only downside it has is that it makes it impossible to implement custom From<SomeType> for Number but I guess we don't really want to support source types that are not convertible to f64 too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible yeah, but I think I'd prefer to stick with the macro strategy to avoid blanket impls unnecessarily blocking out other impls.

crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Pauan Pauan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this!

}
}

impl PartialEq<bool> for Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we impl Eq as well?

Also, shouldn't all of these simple methods be #[inline]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed!


impl From<Boolean> for bool {
fn from(b: Boolean) -> bool {
b.value_of()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be implemented in a more optimized way, but we can save that for later.

* Ensure `PartialEq` is implemented from these types to native Rust types
* Implement `From` between these type and native Rust types
* Deprecated `Number::new` and `Boolean::new` to discourage use of the
  object forms, recommending the `from` constructors instead.

Closes rustwasm#1446
@alexcrichton alexcrichton merged commit 529d0bd into rustwasm:master Apr 12, 2019
@alexcrichton alexcrichton deleted the js-sys-tweaks branch April 12, 2019 15:51
@RReverser
Copy link
Member

RReverser commented Apr 12, 2019

@alexcrichton I see that tests are now giving deprecation warnings due to usage of Boolean::new / Number::new.

Maybe it's worth to either remove these tests, update them to use from or mark them with allow(deprecated) to silence these warnings?

@alexcrichton
Copy link
Contributor Author

Ah oops yeah, it should be fine to just #[allow(deprecated)] them

RReverser added a commit to RReverser/wasm-bindgen that referenced this pull request Apr 17, 2019
Constructing boxed primitives was deprecated in rustwasm#1447.

Some tests have been still using these methods, so this PR either updates them to use newly added {primitive}::from implementations or adds `#[allow(deprecated)]` where necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Soft-deprecate Number::new and Boolean::new
3 participants