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

Fix each* methods to return only nil #3826

Merged
merged 4 commits into from
Jan 5, 2017

Conversation

makenowjust
Copy link
Contributor

Related pull request: #3815

In fact, I fixed only two (1 and 2) by this change. This change has no effect for codes in many case.

@asterite
Copy link
Member

asterite commented Jan 3, 2017

@makenowjust Thank you!

There is an easy way to mark that a method always returns nil:

def each : Nil
  # ...
end

Then you don't have to use nil at the end of the method and also you don't have to remember to return nil in every branch. And this will show up in docs. What do you think?

@makenowjust
Copy link
Contributor Author

@asterite Good. I'll rewrite it in this way.

@makenowjust
Copy link
Contributor Author

@asterite Done.

And, I fixed Int#times and some methods to return nil.

@makenowjust
Copy link
Contributor Author

Hmm...

I think to of Int#upto and Int#downto's arg is bad naming, n is better.

@Sija
Copy link
Contributor

Sija commented Jan 4, 2017

@makenowjust yep, one can use def downto(to n) keep the best from both worlds :)

@asterite
Copy link
Member

asterite commented Jan 4, 2017

@makenowjust I fixed the conflicts after I merged another PR but it seems I did it wrong in one place. Let's keep the name as to for now, later we can rename that to n for to, upto and downto

@asterite asterite added this to the 0.20.4 milestone Jan 5, 2017
@asterite asterite merged commit 6256f77 into crystal-lang:master Jan 5, 2017
@makenowjust makenowjust deleted the fix/each-return-nil branch January 5, 2017 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants