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.

Sign up for GitHub

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
+12 −1
Merged

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

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
966cdf0
range(start, stop; length, step)
Nosferican Oct 3, 2018
f63c02c
Addressed comments
Nosferican Oct 4, 2018
16b7b64
Fix README and Compat from Julia 0.7 - 1.1
Nosferican Oct 4, 2018
021cfe6
Tests for ≥ 0.7
Nosferican Oct 4, 2018
4071b02
Avoid using internal
Nosferican Oct 4, 2018
025c138
Is this the way?
Nosferican Oct 5, 2018
fbc52c3
Fixes.
Nosferican Oct 5, 2018
5f7fd09
fix import statement
Nosferican Oct 5, 2018
e5f549b
Fix tests
Nosferican Oct 5, 2018
8d8906c
Tests and README
Nosferican Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Compat.jl
View file
Original file line number Diff line number Diff line change
@@ -1839,6 +1839,12 @@ if VERSION < v"0.7.0-beta2.143"
end
end

if v"1.0" ≤ VERSION < v"1.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I rebased it just now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# https://github.com/JuliaLang/julia/pull/28708
Base.range(start, stop; length::Union{Integer,Nothing}=nothing, step=nothing) =
Base._range(start, step, stop, length)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

include("deprecated.jl")

end # module Compat
5 changes: 5 additions & 0 deletions test/runtests.jl
View file
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@test range(0, 5, length = 6) == 0.0:1.0:5.0
end

nothing