Deprecate a couple more find(next|prev) methods, and remove ::Integer on start
#25468
Conversation
| @deprecate findfirst(A, v) findfirst(equalto(v), A) | ||
| @deprecate findprev(A, v, i) findprev(equalto(v), A, i) | ||
| @deprecate findlast(A, v) findlast(equalto(v), A) | ||
| @deprecate findnext(A::BitArray, v, i) findnext(equalto(v), A, i) |
There was a problem hiding this comment.
Are these two deprecations really needed, given that we have more general versions above?
There was a problem hiding this comment.
Probably not, though I added them to ensure (mostly) the same specificity we have now. Sort of like your argument above about not removing ::Integer from start.
| findfirstnot(B::BitArray) = findnextnot(B,1) | ||
|
|
||
| # returns the index of the first matching element | ||
| function findnext(B::BitArray, v, start::Integer) |
There was a problem hiding this comment.
I suspect we should keep these for efficiency, but change the signature to findnext(pred::EqualTo, B::BitArray, start). Then inside the function you need to do v = pred.x.
There was a problem hiding this comment.
Isn't that mostly covered by this method? The extra code is O(1).
There was a problem hiding this comment.
Yeah, maybe. So you think there was no reason for the existence of these methods in the first place?
| @deprecate findprev(A, v, i::Integer) findprev(equalto(v), A, i) | ||
| @deprecate findlast(A, v) findlast(equalto(v), A) | ||
| @deprecate find(x::Number) find(!iszero, x) | ||
| @deprecate findnext(A, v, i) findnext(equalto(v), A, i) |
There was a problem hiding this comment.
You should probably keep ::Integer for deprecations, else they are called incorrectly and make tests fail.
There was a problem hiding this comment.
Yeah, that was what I was worried about. I'm abandoning the idea of providing a sentinel argument, so keeping this ::Integer isn't so bad anymore.
There was a problem hiding this comment.
Note that these are just deprecations, we don't need to support new features for them anyway.
There was a problem hiding this comment.
I was worried about deprecations causing ambiguities.
|
I'm going to close this in favor of #25472, but I'll leave the branch around for a bit in case you want it for the |
This deprecates a couple of more methods as part of #23812. It's a small standalone PR to help minimize conflicts between multiple big PRs.