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

range(start, stop; length, step) #633

Merged
merged 10 commits into from Oct 12, 2018

Conversation

4 participants
@Nosferican
Copy link
Contributor

commented Oct 3, 2018

Pre-Compat of #631 per JuliaLang/julia#28708.

@@ -1839,6 +1839,12 @@ if VERSION < v"0.7.0-beta2.143"
end
end

if v"1.0" ≤ VERSION < v"1.1"

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Can't we support this on earlier versions, too? Also, we probably should either wait until JuliaLang/julia#28708 has been merged and we know the exact upper bound, or make this depend on whether the new method is defined. (By looking at methods(range, Tuple{Any,Any}), maybe?)

This comment has been minimized.

Copy link
@Nosferican

Nosferican Oct 4, 2018

Author Contributor

Letting go by JuliaLang/julia#28708 (comment) since it is has already been decided the feature will be added in the next minor release. I can test the @isdefined method as well.

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

it is has already been decided the feature will be added in the next minor release

Yes, but we'd need the XXX in v"1.1.0-DEV.XXX" for the version bound here.

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Oct 4, 2018

Member

I see no reason to merge it now then, why wait?

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

"it" referring to JuliaLang/julia#28708? Agree.

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Oct 4, 2018

Member

Right, I rebased it just now.

This comment has been minimized.

Copy link
@Nosferican

Nosferican Oct 4, 2018

Author Contributor

If we can have it merged now (no more planned patch releases), I can extract the exact version to use here after it has been merged.

@martinholters
Copy link
Member

left a comment

Needs a README entry.

if v"1.0" ≤ VERSION < v"1.1"
# https://github.com/JuliaLang/julia/pull/28708
Base.range(start, stop; length::Union{Integer,Nothing}=nothing, step=nothing) =
Base._range(start, step, stop, length)

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

I don't think we should be calling this internal function. Why not use the old range method?

This comment has been minimized.

Copy link
@Nosferican

Nosferican Oct 4, 2018

Author Contributor

I took it from the un merged PR, but good point.

This comment has been minimized.

Copy link
@Nosferican

Nosferican Oct 4, 2018

Author Contributor

Calling the lowering function does make the code simpler than the various cases using the current range method.

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Doesn't just Base.range(start, stop; kwargs...) = range(start; stop=stop, kwargs...) work?

@@ -1518,4 +1518,9 @@ end
3 4 3 4 3 4]
@test repeat([1, 2], 1, 2, 3) == [x for x in 1:2, y in 1:2, z in 1:3]

# [1.0, 1.1)
if v"1.0" ≤ VERSION < v"1.1"

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Even if we only define the method here prior to v"1.1", we should still test it afterwards to make sure it works as promised.

This comment has been minimized.

Copy link
@Nosferican

Nosferican Oct 4, 2018

Author Contributor

Sure.

Show resolved Hide resolved test/runtests.jl Outdated
@@ -1518,4 +1518,10 @@ end
3 4 3 4 3 4]
@test repeat([1, 2], 1, 2, 3) == [x for x in 1:2, y in 1:2, z in 1:3]

# [1.0, 1.1)
if v"1.0" ≤ VERSION && isempty(methods(range, Tuple{Any,Any}))

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

The isempty(methods(...)) test is wrong here. We definitely want the tests to run if the method is defined. Actually, the tests should (implicitly) verify the method is defined.

This comment has been minimized.

Copy link
@Nosferican

Nosferican Oct 4, 2018

Author Contributor
julia> VERSION
v"1.0.1"

julia> isempty(methods(range, Tuple{Any,Any}))
true

julia> Base.range(start, stop; length::Union{Integer,Nothing}=nothing, step=nothing) =
                  Base._range(start, step, stop, length)

julia> isempty(methods(range, Tuple{Any,Any}))
false

I used that to see if it would test for the missing method. Could you point me to in the direction I should go?

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Just v"1.0" ≤ VERSION? (Although I repeat that I'd prefer this to work from 0.6 on, and I see absolutely no reason why this shouldn't work on 0.7.)

README.md Outdated
@@ -358,6 +358,9 @@ Currently, the `@compat` macro supports the following syntaxes:

* `repmat` is now `repeat` ([#26039])

* `range` will support `range(start, stop; length, step)` for Julia 1.0 which
will be included in Julia 1.1.

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Using future tense is not very ... future-proof here. Maybe just "range supports range(start, stop; length, step) for Julia 1.0" (or "supports stop as a positional argument", although that is less clear). Also should link to the Julia PR.

@@ -1518,4 +1518,10 @@ end
3 4 3 4 3 4]
@test repeat([1, 2], 1, 2, 3) == [x for x in 1:2, y in 1:2, z in 1:3]

# [1.0, 1.1)
if v"0.7" ≤ VERSION && isempty(methods(range, Tuple{Any,Any}))

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Still has the bogus && isempty(methods(range, Tuple{Any,Any})).

This comment has been minimized.

Copy link
@Nosferican

Nosferican Oct 4, 2018

Author Contributor

What replacement do you recommend given that the PR hasn't been merged (can't use a precise v"1.0-xxxx")?

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

None. We want the tests to be run on all versions (0.7 and up, actually). If you look at the other tests, they don't come with an upper version bound. If Julia is recent enough that Compat doesn't have to do anything, we still test things work as expected/promised.

This comment has been minimized.

Copy link
@Nosferican

Nosferican Oct 4, 2018

Author Contributor

So that would be

if v"0.7" ≤ VERSION

then?

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Yes. (That's what I tried to say in #633 (comment)).

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 5, 2018

Member

The condition should be completely removed now (assuming we succeed in making this work for 0.6).

Nosferican added some commits Oct 4, 2018

@@ -1839,6 +1839,11 @@ if VERSION < v"0.7.0-beta2.143"
end
end

if v"0.7" ≤ VERSION

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Ah, this should be guarded with && isempty(methods(range, Tuple{Any,Any})), while the tests should not. However, on 0.7, that does not work, because of the deprecated, old range(a, len) methods. That also makes me wonder whether we actually should be adding this method on 0.7. I'd say yes, but might be worth discussing.

@@ -1839,6 +1839,11 @@ if VERSION < v"0.7.0-beta2.143"
end
end

if v"0.7" ≤ VERSION
# https://github.com/JuliaLang/julia/pull/28708
Base.range(start, stop; kwargs...) = range(start; stop=stop, kwargs...)

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 4, 2018

Member

Heh, thanks to #511, this definition actually works on 0.6, too. But there we have the keyword-argument-less, two-argument methods, too. However, if we omit the Base qualification, then this will add a method to the existing Compat.range on 0.6 and to Base.range on 0.7 and later. Doesn't sound too bad.

@martinholters

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

So condensing my comments, this definition might be worth a try:

if VERSION ≤ v"1.0" || isempty(methods(range, Tuple{Any,Any}))
    range(start, stop; kwargs...) = range(start; stop=stop, kwargs...)
end

However, that would require the code from #511 to be adjusted to only import range after the removal of the deprecated two-arg methods, and define Compat.range(start; kwargs...) = Base.range(start; kwargs...) (or similar) otherwise. Then the new method would be accessible as Compat.range (and just range on 1.0 and above, though I'm not sure we want to advertise that).

@martinholters
Copy link
Member

left a comment

The custom range function has to stay for Julia 0.6. For Julia 0.7, we define our own function which just forwards to Base.range for range(start; kws...) and add a method for range(start, stop; kws...). For Julia 1.0, we add that method to Base.range. I hope that works...

@@ -1506,27 +1506,7 @@ end
const LinRange = Base.LinSpace
export LinRange

function range(start; step=nothing, stop=nothing, length=nothing)

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 5, 2018

Member

This definition needs to stay for 0.6.

Show resolved Hide resolved src/Compat.jl
@@ -1527,6 +1527,9 @@ end
return linspace(start, stop, length)
end
end
elseif VERSION < v"1.0.0-DEV.57"
import LinRange

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 5, 2018

Member

Sorry, this should be import Base: LinRange.

Nosferican added some commits Oct 5, 2018

@@ -1518,4 +1518,8 @@ end
3 4 3 4 3 4]
@test repeat([1, 2], 1, 2, 3) == [x for x in 1:2, y in 1:2, z in 1:3]

# Support for positional `stop`
@test range(0, 5, length = 6) == 0.0:1.0:5.0
@test range(0, 10, step = 2) == 0:2:10

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 5, 2018

Member

These should be Compat.range.

README.md Outdated
@@ -358,6 +358,8 @@ Currently, the `@compat` macro supports the following syntaxes:

* `repmat` is now `repeat` ([#26039])

* `range` supports `range(start, stop; length, step)` (`stop` as positional argument) ([#28708])

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 5, 2018

Member

This should be Compat.range.

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 5, 2018

Member

Hm, could also be merged with

* `Compat.range` supporting keyword arguments ([#25896]).

but I'm not sure of a good wording.

@martinholters
Copy link
Member

left a comment

Good to go from my side. I'd like to keep it open for a bit, though, for others to have chance to review the final version we've come up with here.

Also might be worth testing against https://github.com/JuliaLang/julia/tree/jb/2argrange to make sure it doesn't break once JuliaLang/julia#28708 gets merged. Have you done that? Otherwise I can do so.

@Nosferican

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

I haven't. I would appreciate if you can check against it and let me know (I mostly use the binaries and don't build it from source).

@arnavs arnavs referenced this pull request Oct 5, 2018

Closed

Ranges pass #149

@arnavs

This comment has been minimized.

Copy link

commented Oct 11, 2018

Any progress on this?

@martinholters

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Thanks for the bump, I had gotten distracted by other things. So I've successfully checked against JuliaLang/julia#28708 just now, nobody has voiced any objections... Merge!

@martinholters martinholters merged commit 052636a into JuliaLang:master Oct 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arnavs

This comment has been minimized.

Copy link

commented Oct 16, 2018

Thanks so much @martinholters! And appreciate your tagging a new release so we can use the feature.

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