Fix #830 .step() .upto() .downto() and .to() arguments #3784
Conversation
|
@schovi Yes, the arguments should be renamed :-) |
|
@asterite looks like we have hanging specs again. |
| @@ -297,7 +297,7 @@ describe "Int" do | |||
| describe "step" do | |||
| it "steps through limit" do | |||
| passed = false | |||
| 1.step(1) { |x| passed = true } | |||
| 1.step(by: 1) { |x| passed = true } | |||
There was a problem hiding this comment.
This is the test that hangs travis. This should be to: 1, otherwise it iterates infinitely
There was a problem hiding this comment.
This would have been caught by overflow detection, would it not?
schovi
force-pushed
the
normalize-step-arguments
branch
2 times, most recently
from
de9afc2 to
af83503
Compare
schovi
changed the title
schovi
force-pushed
the
normalize-step-arguments
branch
2 times, most recently
from
d2976e4 to
b40773d
Compare
schovi
force-pushed
the
normalize-step-arguments
branch
from
b40773d to
b1e7ff2
Compare
|
Thank you! |
|
Note that adding required named arguments in I am not saying we should change something, just pointing it out for future demos. cc: @asterite. We might need to add an new entry to https://github.com/crystal-lang/crystal/wiki/Crystal-for-Rubyists . |
|
@bcardiff I changed samples, which failed during CI/tests. Why this doesn't fail? |
|
@schovi there is nothing wrong with the changes, it's just that sometimes we have demonstrated the mandelbrot.cr sample using crystal and ruby without changing the code. That time is over since this change. It's just a heads up for future demos. |
|
@bcardiff Ah. Didn't get it first time :) |
Renaming of arguments for
.step()function as mentioned in #830[1,2,3,4,5].each.step([by:]2)2.step(to: 10, by: 2)- required named argumentsFor discussion: I was thinking about rename arguments for methods like
.upto()in same manner..upto(n: 2)will be.upto(to: 2). What do you think?