Add zero? to Number, Time::Span, and Complex#4026
Add zero? to Number, Time::Span, and Complex#4026bcardiff merged 5 commits intocrystal-lang:masterfrom
zero? to Number, Time::Span, and Complex#4026Conversation
|
But...why? |
|
@kirbyfan64 it's a Rubyism. |
|
The only benefit I see in ruby is to use it in a map / filter. But in crystal it is possible to do [1,0].map &.==(0) # => [false, true] |
|
Not as pretty as |
|
I think it's more "human readable" e.g. as opposed to I ran into it while converting some Ruby code into Crystal so I thought I'd add it. It also might save some Rubyists from a few surprises as well. EDIT I agree with @ysbaddaden. IMO it's prettier to write |
|
@jellymann There is Crystal land, so you need to learn Crystal culture. And, I don't think |
zero? to Numberzero? to Number and Time::Span
zero? to Number and Time::Spanzero? to Number, Time::Span, and Complex
src/time/span.cr
Outdated
| end | ||
|
|
||
| def zero? | ||
| self == typeof(self).zero |
There was a problem hiding this comment.
why not simply @ticks == 0?
There was a problem hiding this comment.
Read @makenowjust's comment. This is a more general approach, comparing directly to the type's definition of "zero"
There was a problem hiding this comment.
This "more general approach" needlessly allocates new object on the stack, while actually you only need to compare the @ticks ivar here. There's a reasoning behind it in relation to Number since it may hold different values not always comparable with 0 — as Int32. It does not apply here at all though.
There was a problem hiding this comment.
If it's a general implementation, then it should be on a more general type.
There was a problem hiding this comment.
I don't see how Time::Span is a general type. There's Time::Span::Zero constant more suitable for this anyhow.
There was a problem hiding this comment.
@Sija I was agreeing with you.
self == typeof(self).zero is a general implementation because it works on all types with a self.zero implementation. I was saying that there's no point in using a more general implementation like this versus @ticks == 0, unless you move #zero? to Object.
There was a problem hiding this comment.
Okay, so there's no point in doing self == typeof(self).zero if it's not on a general type. Number is a general type, so it's probably okay to leave it. Time::Span and Complex could probably be more specific, though, since they're concrete types.
There was a problem hiding this comment.
Unless we create a Zeroable module that we just include on every type that has a self.zero implementation? That also allows people to juste drop in Zeroable on their own classes if they need to.
e.g.
class Vector3
def self.zero
new 0, 0, 0
end
include Zeroable
endThere was a problem hiding this comment.
That could work, although it seems a bit verbose to have a module just for that.
src/number.cr
Outdated
| # 5.zero? #=> false | ||
| # ``` | ||
| def zero? : Bool | ||
| self == typeof(self).zero |
There was a problem hiding this comment.
why not simply self.to_f == 0?
There was a problem hiding this comment.
Read @makenowjust's comment. This is a more general approach, comparing directly to the type's definition of "zero"
There was a problem hiding this comment.
For struct Number is ok the typeof(self).zero since the literal expanded would be of exact type and that is better. But on Complex and Timespan I would say to go for the plain/@ivar == approach. The compiler will end up inlining and simplifying it, but I think is simpler the @ivar == and we avoid the extra work in the optimization.
|
@kirbyfan64 Is Crystal's goal to save keystrokes? You’d think that a language inspired by Ruby would actively embrace Rubyisms. |
|
We don't want to copy every rubyism. For example we avoid aliases and prefer a single way or method to do something. To add a new feature into the core/stdlib we need to make sure it has a real, added, value. |
|
One benefit of this would be slightly more type safety because in |
|
Since using Yet, if an implementation is supposed to receive a |
|
I'd just like to say, another reason why I think this is a good fit is that Also, |
|
I also think |
|
Bump, what's the state of this PR? (the build error is about file formatting) |
|
I vote for this pull request. Zero has special meaning, and need Formatting should be updated due to changes in crystal formatter itself. |
|
@jellymann are you available to fix the formatting so this can be merged? |
7564d1b to
3502fec
Compare
Removed generic implementation of `zero?` to avoid unecessary optimisation.
3502fec to
abc10ec
Compare
|
@bcardiff I've fixed the formatting issues, but the Travis build is still failing for some reason. |
|
Thanks @jellymann! |
No description provided.