deprecate constructors with type as first argument #34
Conversation
|
Tests fail because of JuliaLang/Compat.jl#416 (comment), so need to wait for Compat v0.39. |
fredrikekre
force-pushed
the
fe/type-constructor
branch
from
5d8692e
to
30d121a
Compare
src/OffsetArrays.jl
Outdated
| OffsetArray(f(map(length, shape)), map(indexoffset, shape)) | ||
| Base.similar(::Type{T}, shape::Tuple{UnitRange,Vararg{UnitRange}}) where {T<:OffsetArray} = | ||
| OffsetArray(T(map(length, shape)), map(indexoffset, shape)) |
There was a problem hiding this comment.
I had to split this into two methods to fix this on julia master:
julia> Test.detect_ambiguities(OffsetArrays, Base, Core)
Skipping Base.Date
2-element Array{Tuple{Method,Method},1}:
(similar(f::Union{Function, Type}, shape::Tuple{UnitRange,Vararg{UnitRange,N} where N}) in OffsetArrays at /Users/Fredrik/.julia/v0.7/OffsetArrays/src/OffsetArrays.jl:89, similar(::Type{T}, shape::Tuple) where T<:BitArray in Base at bitarray.jl:355)
(similar(f::Union{Function, Type}, shape::Tuple{UnitRange,Vararg{UnitRange,N} where N}) in OffsetArrays at /Users/Fredrik/.julia/v0.7/OffsetArrays/src/OffsetArrays.jl:89, similar(::Type{T}, shape::Tuple) where T<:Array in Base at array.jl:254)
src/OffsetArrays.jl
Outdated
| OffsetArray(f(map(length, shape)), map(indexoffset, shape)) | ||
| Base.similar(::Type{T}, shape::Tuple{UnitRange,Vararg{UnitRange}}) where {T<:OffsetArray} = | ||
| OffsetArray(T(map(length, shape)), map(indexoffset, shape)) |
There was a problem hiding this comment.
Will the T version trigger your deprecation warning?
|
Yes there is some more stuff to work out here, see e.g. JuliaLang/julia#24785 (comment) I'm planning to take a look tonight. |
src/OffsetArrays.jl
Outdated
| Base.similar(::Type{T}, shape::Tuple{UnitRange,Vararg{UnitRange}}) where {T<:Array} = | ||
| OffsetArray(T(map(length, shape)), map(indexoffset, shape)) | ||
| Base.similar(::Type{T}, shape::Tuple{UnitRange,Vararg{UnitRange}}) where {T<:BitArray} = | ||
| OffsetArray(T(map(length, shape)), map(indexoffset, shape)) |
There was a problem hiding this comment.
These two similar methods make me a bit uncomfortable.
There was a problem hiding this comment.
What motivated their addition? Presumably a test failure?
|
Tests pass locally with JuliaLang/julia#24918 now, but not sure about those |
fredrikekre
force-pushed
the
fe/type-constructor
branch
2 times, most recently
from
c5c84d6
to
33277f4
Compare
fredrikekre
force-pushed
the
fe/type-constructor
branch
from
33277f4
to
c80b595
Compare
|
Rebased after #37 |
| OffsetVector{T}(inds::AbstractUnitRange) where {T} = OffsetArray{T}(inds) | ||
|
|
||
| # https://github.com/JuliaLang/julia/pull/19989 | ||
| Base.@deprecate OffsetArray(::Type{T}, inds::Vararg{UnitRange{Int},N}) where {T,N} OffsetArray{T}(inds) |
There was a problem hiding this comment.
What about switching to uninitialized syntax now? We're moving now to ArrayType(iterator) constructs the array from the contents of iterator, but since -3:3 is an iterator it's ambiguous whether that's supposed to be treated as the initializing values or the axes.
There was a problem hiding this comment.
I have that change as another PR in the pipeline, I can include it in this one if you prefer?
There was a problem hiding this comment.
Either way is fine with me. I'll merge this and you can do that in a next one.
This type of
Arrayconstructors where deprecated in JuliaLang/julia#19989, soOffsetArraysshould do the same. Including the comma and the space, the replacement is even the same number of characters 😄