Deprecate slicedim(…) to copy(selectdim(…)); always return a view #26009
Conversation
mbauman
added
arrays
|
I didn't even know this function existed; we don't use it anywhere in Base, but it seems to be used in a dozen or so packages. I wonder if there should be a |
|
Yeah, see my comments at #18326 (comment) — one package defines its own This function is so old (1041bb4) I'd bet it predates views. |
base/deprecated.jl
Outdated
| @@ -1360,6 +1360,9 @@ end | |||
| # issue #25928 | |||
| @deprecate wait(t::Task) fetch(t) | |||
|
|
|||
| # PR #26008 | |||
| @deprecate slicedim(A::AbstractArray, d::Integer, i) selectdim(A, d, i) | |||
There was a problem hiding this comment.
If it's a straight renaming of one to the other you can just use @deprecate slicedim selectdim
There was a problem hiding this comment.
Yeah, I suppose it's effectively the same — that would define and export a broader signature, but it'd still hit a method error later on.
|
You know what, I think views are indeed the better functionality here… and a name change is a great time to do that. I pushed a new commit for that. |
mbauman
changed the title
base/deprecated.jl
Outdated
| @@ -26,7 +26,7 @@ macro deprecate(old, new, ex=true) | |||
| newname = Expr(:quote, new) | |||
| Expr(:toplevel, | |||
| ex ? Expr(:export, esc(old)) : nothing, | |||
| :(function $(esc(old))(args...) | |||
| :(@__doc__ function $(esc(old))(args...) | |||
There was a problem hiding this comment.
Watch out, I snuck in the ability to document deprecated functions :)
There was a problem hiding this comment.
Oh, never mind, it bails on bootstrapping and I don't feel strongly enough to battle that one out.
mbauman
force-pushed
the
mb/selectdim
branch
from
7b42c00 to
c5d58dd
Compare
NEWS.md
Outdated
| @@ -646,6 +646,10 @@ Deprecated or removed | |||
| * Using Bool values directly as indices is now deprecated and will be an error in the future. Convert | |||
| them to `Int` before indexing if you intend to access index `1` for `true` and `0` for `false`. | |||
|
|
|||
| * `slicedim(A, d, i)` has been deprecated in favor of `copy(selectdim(A, d, i)`. The new | |||
Add support for SubArrays of BitArrays in BitArray's test suite
|
Ok, I think I should have tests passing now. Interestingly, always returning a view and moving to a new name makes #20233 non-breaking. |
mbauman
dismissed
ararslan’s stale review
functionality changed since review
|
Given that it always returns a view now, maybe it should be called |
Fixes #18326.
Edit: this now couples a behavior change in with the name change. The deprecation gives us a convenient time to improve its behavior to use views instead of getindex.