-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
} | ||
)*) | ||
} | ||
number_from!(i8 u8 i16 u16 i32 u32 f32 f64); |
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.
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?
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.
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.
59f9b22
to
89b02fe
Compare
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 like this!
} | ||
} | ||
|
||
impl PartialEq<bool> for Boolean { |
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.
Shouldn't we impl Eq
as well?
Also, shouldn't all of these simple methods be #[inline]
?
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.
Indeed!
|
||
impl From<Boolean> for bool { | ||
fn from(b: Boolean) -> bool { | ||
b.value_of() |
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.
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
89b02fe
to
ba88ae8
Compare
@alexcrichton I see that tests are now giving deprecation warnings due to usage of Maybe it's worth to either remove these tests, update them to use |
Ah oops yeah, it should be fine to just |
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.
PartialEq
is implemented from these types to native Rust typesFrom
between these type and native Rust typesNumber::new
andBoolean::new
to discourage use of theobject forms, recommending the
from
constructors instead.Closes #1446