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

Add missing `String#to_i` type annotations #7436

Merged

Conversation

@j8r
Copy link
Contributor

commented Feb 15, 2019

No description provided.

@r00ster91

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Why are they missing? Also what's with the underscore argument?

@asterite

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

What do you mean by missing? Type annotations are optional. This is a breaking change.

@j8r

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

Yeah I miss the underscore, thanks @r00ster91

@j8r j8r force-pushed the j8r:add-missing-to_i-type-annotations branch from 99c9ebd to 364d05d Feb 15, 2019

@j8r

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

By missing, I mean they are inferred to booleans - but their types are missing in the docs.

src/string.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
@asterite

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

They are not inferred to booleans. With this change one cannot pass nil or any truthy value other than true anymore. This is why this is a breaking change.

It's not that I'm against this change, though... but we have to be careful about merging this.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

I think it's better to only allow Bool instead of any truthy or falsey values.

@j8r j8r force-pushed the j8r:add-missing-to_i-type-annotations branch 2 times, most recently from 83c7c57 to e11ff33 Feb 15, 2019

@j8r j8r force-pushed the j8r:add-missing-to_i-type-annotations branch from e11ff33 to 1612543 Mar 30, 2019

@j8r

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Anything blocking for merging?

@j8r j8r force-pushed the j8r:add-missing-to_i-type-annotations branch from 1612543 to 7df3a4e May 3, 2019

@asterite asterite added this to the 0.29.0 milestone May 3, 2019

@asterite asterite merged commit c740506 into crystal-lang:master May 3, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@j8r j8r deleted the j8r:add-missing-to_i-type-annotations branch May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.