Remove remark about poor code style #37503
Conversation
The current wording [seems to be confusing](https://www.reddit.com/r/rust/comments/5aat03/why_is_implementing_traits_on_primitive_types/). As an explanation when and why this could be considered as poor style would go beyond of the scope of this chapter I suggest to remove this remark.
|
r? @Manishearth (rust_highfive has picked a reviewer for you, use r? to override) |
|
|
||
| It is considered poor style to implement methods on such primitive types, even | ||
| though it is possible. | ||
| implement a trait for any type such as `i32`. |
There was a problem hiding this comment.
leave the example in I guess?
There was a problem hiding this comment.
Maybe. Then I would suggest to change the implementation to
impl HasArea for i32 {
fn area(&self) -> f64 {
println!("this is silly");
0.0
}
}to depict the silliness of calculating the area of a scalar.
There was a problem hiding this comment.
That would actually be bad style (imho). One cannot implement a meaningful area for i32, only for the newtype struct Square(i32). This is why I wanted to remove the example altogether because that possibly lengthy discussion is out of scope of that particular paragraph.
There was a problem hiding this comment.
@nwin Can you suggest an alternative non-silly example that we could put here?
There was a problem hiding this comment.
We could make up a trait like Complex and output the real and imaginary part in two methods.
There was a problem hiding this comment.
I've just come up with a better idea.
trait ApproxEqual {
fn approx_equal(&self, other: &Self);
}
impl ApproxEqual for f64 {
fn approx_equal(&self, other: &Self) {
// Impementation to be improved for the actual example
// Only valid for numbers close to 1.0
(self - other).abs() < ::std::f64::EPSILON
}
|
r? @steveklabnik who wrote this I think |
|
I did write this, but I really think it's still unclear if this kind of @rust-lang/docs thoughts here? Also maybe @rust-lang/libs. On Mon, Oct 31, 2016 at 3:01 PM, Manish Goregaokar <notifications@github.com
|
|
That's a subject which comes often lately. So, I'm for a rewriting, not a removal. |
|
I’ve updated the example. |
| let diff = (self - other).abs(); | ||
| let (a, b) = (self.abs(), other.abs()); | ||
| let largest = if a > b { a } else { b }; | ||
| if diff <= largest * ::std::f32::EPSILON { |
There was a problem hiding this comment.
You can remove the if and { true } else { false } and get the same result.
|
@steveklabnik I may be missing the context here, but I wouldn't consider it poor style to implement traits for primitive types. Rather, implementing traits for primitive types is great as you can enhance what they can do. Is this referring to something specific, though? Such as implementing inappropriate traits for integers? (e.g. what does it mean for an In any case looks like there aren't many other opinions, so thoughts about approving/closing @steveklabnik? |
alexcrichton
added
the
A-docs
|
I think the poor style comment may source from Rust's general avoidance of Ruby-style things like |
|
@alexcrichton as I said on reddit: Hey there! I wrote this, so figure I should chime in. So, the first thing I'll say is this: what's 'good style' and not can evolve over time. I wrote this text in December of 2014, almost two years ago. When I wrote it, I remembered writing stuff like Rust is surprisingly expressive, which got a really, really bad reception. People really don't like putting DSL-like methods on primitive types. That said, sometimes, some traits make sense. It just depends, this isn't a super hard-and-fast rule. The points people make in this thread pretty much cover the debate. |
| impl ApproxEqual for f32 { | ||
| fn approx_equal(&self, other: &Self) -> bool { | ||
| // Approximately correct implementation. See also: | ||
| // https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/ |
There was a problem hiding this comment.
linking to external URLs like this is generally discouraged; we try not to even link to wikipedia.
There was a problem hiding this comment.
we might not even really need to implement a body here, given that it's really not about how this is done.
|
I feel 50/50 about it. The original text may be a bit too strongly worded, and have a bad example, but this example is fine. so i guess i'm leaning towards merge |
|
@steveklabnik ah ok thanks for clarifying! Sounds good to me! |
|
@rust-lang/docs what do you think about my two review comments here? |
|
I'll said about external urls might sound a bit sad but:
I don't have much opinion on @steveklabnik's second point except maybe that the example sounds more complicated that it should. |
|
Ok, points taken, I simplified the example such that the describing comment can be kept simple. |
|
@bors: r+ rollup thank you so much! |
|
📌 Commit 5cf07f1 has been approved by |
Remove remark about poor code style The current wording [seems to be confusing](https://www.reddit.com/r/rust/comments/5aat03/why_is_implementing_traits_on_primitive_types/). As an explanation when and why this could be considered as poor style would go beyond of the scope of this chapter I suggest to remove this remark.
Remove remark about poor code style The current wording [seems to be confusing](https://www.reddit.com/r/rust/comments/5aat03/why_is_implementing_traits_on_primitive_types/). As an explanation when and why this could be considered as poor style would go beyond of the scope of this chapter I suggest to remove this remark.
Rollup of 30 pull requests - Successful merges: #37190, #37368, #37481, #37503, #37527, #37535, #37551, #37584, #37600, #37613, #37615, #37659, #37662, #37669, #37682, #37688, #37690, #37692, #37693, #37694, #37695, #37696, #37698, #37699, #37705, #37708, #37709, #37716, #37724, #37727 - Failed merges: #37640, #37689, #37717
The current wording seems to be confusing. As an explanation when and why this could be considered as poor style would go beyond of the scope of this chapter I suggest to remove this remark.