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

Int div deprecation #7639

Merged
merged 2 commits into from
Apr 12, 2019
Merged

Conversation

bcardiff
Copy link
Member

@bcardiff bcardiff commented Apr 5, 2019

Depends on #7638.

This PR replaces (hopefully) all usages of Int#/ with Int#//.

Int#/ is marked as deprecated. This will cause some noise while building the compiler and specs since the method is still tested and used in places like macro interpreter. Without turning off warnings on some lexical scope we will need to tolerate that noise for a bit.

Since the stdlib is not ignored bye defualt (only ./lib) if there are some functions without coverage in the stdlib users will notice them and will be able to report :-)

Ref: #2968


Submitting as draft until #7638 is merged

@bcardiff bcardiff added this to the 0.28.0 milestone Apr 5, 2019
@bcardiff bcardiff force-pushed the feature/int-div-deprecation branch from 10ce45c to 1c2b853 Compare April 10, 2019 22:32
@bcardiff bcardiff marked this pull request as ready for review April 10, 2019 22:34
@bcardiff
Copy link
Member Author

Ready for the review!

src/int.cr Outdated Show resolved Hide resolved
@bcardiff bcardiff force-pushed the feature/int-div-deprecation branch from 1c2b853 to 30bda77 Compare April 11, 2019 13:25
@bew
Copy link
Contributor

bew commented Apr 11, 2019

What about a more imperative style:

Use Int#// for integer division.

Instead of:

Might need to use Int#// for integer division.

src/crystal/hasher.cr Outdated Show resolved Hide resolved
In 0.29.0:
Number#/ will mean arithmetic division
Number#// will mean integer (and floor) division
@bcardiff bcardiff force-pushed the feature/int-div-deprecation branch from 30bda77 to 32d7654 Compare April 12, 2019 15:13
@bcardiff
Copy link
Member Author

Both suggestions addressed.

@j8r
Copy link
Contributor

j8r commented Apr 12, 2019

I have a problem: Int#/ isn't deprecated, it behaves differently.
It's possible to be needlessly spammed for a very valid code, like @RX14 point out.
It should be another type of warning, with a lower priority, disabled by default. information/note maybe.
The deprecated annotation doesn't fit for every API change.

@bcardiff
Copy link
Member Author

@j8r it's disabled by default. When there is a change of the semantic although it's not a formal deprecation you need to call for a review at least. What it is lacking is a way to mark the warning as solved for example in the code. Although I am fine with dealing some false-positive reports.

A fully smooth migration would be to force every / to be either // or temp_float_div and then change temp_float_div to /, but I hardly believe somebody would embrace that journey.

@j8r
Copy link
Contributor

j8r commented Apr 12, 2019

Yes I know warnings are disabled by default @bcardiff. Still, this is misleading to mark Int#/ as deprecated – it's not. This syntax will stay valid, and won't go away (just behave differently).
Deprecated means something that shouldn't be used anymore.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 12, 2019

just behave differently

But that's exactly why its current use needs to be deprecated.

You could just see it like this: Int#/(Int) : Int is going to be removed in 0.29.0. If your code uses this method, it shouldn't. That's what the deprecation warning says.
In the same release (probably, except we make the extra detour @bcardiff described 😆 ) we're going to add Int#/(Int) : Float which essentially takes its place. But it's a different method with a different signature and different behaviour. Code using this method might continue to build or might fail, depending how the return value is used.

@j8r
Copy link
Contributor

j8r commented Apr 12, 2019

The signature change, right. I doubt this really matter anyway. This isn't like this warnings will impact non insiders, unless they are curious about tryings flags or read the changelog. That's fine then.

@bcardiff bcardiff merged commit e361025 into crystal-lang:master Apr 12, 2019
@bcardiff bcardiff deleted the feature/int-div-deprecation branch April 13, 2019 15:36
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.

6 participants