add find(first|last|next|prev) methods that return nothing #513

fredrikekre · 2018-03-08T13:13:40Z

No description provided.

nalimilan

Thanks, I had totally forgotten that we needed Compat for this too!

nalimilan · 2018-03-08T13:24:43Z

src/Compat.jl

@@ -1498,6 +1498,7 @@ else
    export occursin

    zero2nothing(x::Integer) = x == 0 ? nothing : x
+    zero2nothing(x::AbstractUnitRange{<:Integer}) = x == 0:-1 ? nothing : x


Maybe use isempty just in case?

nalimilan · 2018-03-08T13:25:57Z

test/runtests.jl

@@ -1378,18 +1378,14 @@ for (f1, f2, i) in ((Compat.findfirst, Compat.findnext, 1),
        @test f2(occursin(chars), "bx", i) == f1(occursin(chars), "bx") == nothing
    end
 end
-for (f1, f2, i) in ((findfirst, findnext, 1),


I think we should keep testing the unprefix methods as Compat provides a few ones (which do not return nothing).

But they return nothing on 0.7. In fact this block of code does not pass on master.

Ah, OK. Should probably have a version-specific test then, as long as we provide these methods via Base.findfirst, etc.

What is the point in providing Compat methods which do not behave like 0.7 though?

I guess none of

Compat.jl/src/Compat.jl

Lines 1508 to 1540 in fd3b92a

Base.findnext(r::Regex, s::AbstractString, idx::Integer) = search(s, r, idx)

Base.findfirst(r::Regex, s::AbstractString) = search(s, r)

Base.findnext(c::EqualTo{Char}, s::AbstractString, i::Integer) = search(s, c.x, i)

Base.findfirst(c::EqualTo{Char}, s::AbstractString) = search(s, c.x)

Base.findnext(b::EqualTo{<:Union{Int8,UInt8}}, a::Vector{<:Union{Int8,UInt8}}, i::Integer) =

search(a, b.x, i)

Base.findfirst(b::EqualTo{<:Union{Int8,UInt8}}, a::Vector{<:Union{Int8,UInt8}}) =

search(a, b.x)

Base.findnext(c::OccursIn{<:Union{Tuple{Vararg{Char}},AbstractVector{Char},Set{Char}}},

s::AbstractString, i::Integer) =

search(s, c.x, i)

Base.findfirst(c::OccursIn{<:Union{Tuple{Vararg{Char}},AbstractVector{Char},Set{Char}}},

s::AbstractString) =

search(s, c.x)

Base.findnext(t::AbstractString, s::AbstractString, i::Integer) = search(s, t, i)

Base.findfirst(t::AbstractString, s::AbstractString) = search(s, t)

Base.findfirst(delim::EqualTo{UInt8}, buf::Base.IOBuffer) = search(buf, delim.x)

Base.findprev(c::EqualTo{Char}, s::AbstractString, i::Integer) = rsearch(s, c.x, i)

Base.findlast(c::EqualTo{Char}, s::AbstractString) = rsearch(s, c.x)

Base.findprev(b::EqualTo{<:Union{Int8,UInt8}}, a::Vector{<:Union{Int8,UInt8}}, i::Integer) =

rsearch(a, b.x, i)

Base.findlast(b::EqualTo{<:Union{Int8,UInt8}}, a::Vector{<:Union{Int8,UInt8}}) =

rsearch(a, b.x)

Base.findprev(c::OccursIn{<:Union{Tuple{Vararg{Char}},AbstractVector{Char},Set{Char}}},

s::AbstractString, i::Integer) = rsearch(s, c.x, i)

Base.findlast(c::OccursIn{<:Union{Tuple{Vararg{Char}},AbstractVector{Char},Set{Char}}},

s::AbstractString) = rsearch(s, c.x)

Base.findprev(t::AbstractString, s::AbstractString, i::Integer) = rsearch(s, t, i)

Base.findlast(t::AbstractString, s::AbstractString) = rsearch(s, t)

should extend the Base functions. I think we should deprecate those and require Compat.find* such that people have the 0.7 behaviour w.r.t nothing?

I don't get it, now we basically just provide another 0.6 interface to those functions. So the options here are 1. deprecate those to Compat.find* methods or 2. add the same methods but unexported that return nothing. I definitely think we should go with 2. It is very confusing to provide findfirst(...) and Compat.findfirst() with the same signatures, where only one of them have 0.7 behaviour.

Doesn't 2. already work?

Yea you are right, it does, but I still think it is confusing. The Base extended methods are only useful on 0.6, at which point you might use the 0.6 syntax (e.g. with search and rsearch etc.)

Yes, I'm not saying they are really useful, but that deprecating them is more trouble than it's worth.

Fair enough.

stevengj · 2018-03-08T13:27:46Z

README.md

@@ -355,6 +355,10 @@ Currently, the `@compat` macro supports the following syntaxes:
 * `Compat.findfirst`, `Compat.findnext`, `Compat.findlast` and `Compat.findprev`,
  return `nothing` when no match is found (rather than `0`) as on Julia 0.7 ([#24673]).

+* `Compat.findfirst`, `Compat.findnext`, `Compat.findlast` and `Compat.findprev`,
+  return `nothing` when no match is found (rather than the empty range `0:-1`)


If you are searching for a particular element they return 0 on 0.6, not 0:-1.

Methods previously returning 0 were added in #484, see the bullet just above. Perhaps I can collapse them to the same bullet.

Oh, I see. Yes, please combine the bullets.

stevengj · 2018-03-08T13:28:22Z

src/Compat.jl

@@ -1498,6 +1498,7 @@ else
    export occursin

    zero2nothing(x::Integer) = x == 0 ? nothing : x
+    zero2nothing(x::AbstractUnitRange{<:Integer}) = x == 0:-1 ? nothing : x


You could use ifelse(iszero(first(x)), nothing, x) to combine the two methods.

IMHO that would be harder to read for no real advantage. Also I think the compiler is smart enough to avoid branches were it's faster even if you use ? (at least that's what Jameson noted somewhere).

Fine, no big deal either way.

nalimilan · 2018-03-09T10:27:58Z

Woops, sorry, my isempty suggestion was actually incorrect since e.g. findfirst(r"", "a") == 1:0 even on 0.7. I've reverted the change directly.

…513

* Bump required Julia version to 1.0 * Remove compatibility support code for: * `at-__MODULE__` (from #363) * `devnull`, `stdin`, `stdout`, and `stderr` from #499 * `at-nospecialize` (from #385 and #409) * `isabstracttype` and `isconcretetype` (from #477) * `invokelatest` from #424 * array-like access to `Cmd` from #379 * `Val(n)` and `ntuple`/`reshape` with `Val` from #381 and #399 * `logdet(::Any)` fallback from #382 * `chol(::UniformScaling)` from #382 * `pushfirst!`, `popfirst!` from #444 * `fieldcount` from #386 * `read(obj, ::Type{String})` from #385 and #580 * `InexactError`, `DomainError`, and `OverflowError` constructors from #393 * `corrected` kw arg to `cov` from #401 * `adjoint` from #401 * `partialsort` from #401 * `pairs` from #428 * `AbstractRange` from #400 * `rtoldefault` from #401 * `Dates.Period` rounding from #462 * `IterativeEigensolvers` from #435 * `occursin` from #520 * `Char` concatenation from #406 * `BitSet` from #407 * `diagm` and `spdiagm` with pairs from #408 * `Array` c'tors from `UniformScaling` from #412 and #438 * `IOContext` ctor taking pairs from #427 * `undef` from #417 and #514 * `get` on `ENV` from #430 * `ComplexF...` from #431 * `tr` from #614 * `textwidth` from #644 * `isnumeric` from #543 * `AbstractDict` from #435 * `axes` #435 and #442 * `Nothing` and `Cvoid` from #435 * `Compat.SuiteSparse` from #435 * `invpermute!` from #445 * `replace` with a pair from #445 * `copyto!` from #448 * `contains` from #452 * `CartesianIndices` and `LinearIndices` from #446, #455, and #524 * `findall` from #466 (and #467). * `argmin` and `argmax` from #470, #472, and #622 * `parentmodule` from #461 * `codeunits` from #474 * `nameof` from #471 * `GC` from #477 * `AbstractDisplay` from #482 * `bytesavailable` from #483 * `firstindex` and `lastindex` from #480 and #494 * `printstyled` from #481 * `hasmethod` from #486 * `objectid` from #486 * `Compat.find*` from #484 and #513 * `repr` and `showable` from #497 * `Compat.names` from #493 and #505 * `Compat.round` and friends #500, #530, and #537 * `IOBuffer` from #501 and #504 * `range` with kw args and `LinRange` from #511 * `cp` and `mv` from #512 * `indexin` from #515 * `isuppercase` and friends from #516 * `dims` and `init` kwargs from #518, #528, #590, #592, and #613 * `selectdim` from #522 and #531 * `repeat` from #625 * `fetch(::Task)` from #549 * `isletter` from #542 * `isbitstype` from #560 * `at-cfunction` from #553 and #566 * `codeunit` and `thisind` and friends from #573 * `something` from #562 * `permutedims` from #582 * `atan` from #574 * `split` and `rsplit` from #572 * `mapslices` from #588 * `floatmin` and `floatmax` from #607 * `dropdims` from #618 * required keyword arguments from #586 * `CartesianRange` in `at-compat` from #377 * `finalizer` from #416 * `readline`, `eachline`, and `readuntil` from #477, #541, and #575 * curried `isequal`, `==`, and `in` from #517 * `Some` from #435 and #563 * `at-warn` and friends from #458 * Remove old deprecations * Deprecate: * `Compat.Sockets` from #545 and #594 * `TypeUtils` from #304 * `macros_have_sourceloc` from #355 * `Compat.Sys` from #380, #433, and #552 * `Compat.MathConstants` from #401 * `Compat.Test`, `Compat.SharedArrays`, `Compat.Mmap`, and `Compat.DelimitedFiles` from #404 * `Compat.Dates` from #413 * `Compat.Libdl` from #465 (and #467) * `AbstractDateTime` from #443 * `Compat.Printf` from #435 * `Compat.LinearAlgebra` from #463 * `Compat.SparseArrays` from #459 * `Compat.Random` from #460, #601, and #647 * `Compat.Markdown` from #492 * `Compat.REPL` from #469 * `Compat.Serialization` from #473 * `Compat.Statistics` from #583 * `Fix2` from #517 * `Compat.Base64` from #418 * `Compat.Unicode` from #432 and #507 * `notnothing` from #435 and #563 * `Compat.IteratorSize` and `Compat.IteratorEltype` from #451 * `enable_debug(::Bool)` from #458 * `Compat.Distributed` from #477 * `Compat.Pkg` from #485 * `Compat.InteractiveUtils` from #485 * `Compat.LibGit2` from #487 * `Compat.UUIDs` from #490 * `Compat.qr` from #534 * `Compat.rmul!` from #546 * `Compat.norm` abd friends from #577 * Remove obsolete README entry, missed in #385 * Remove obsolete tests (e.g. missed in #372) * Remove obsolete `VERSION` conditionals and some minor clean-up

fredrikekre requested a review from nalimilan March 8, 2018 13:13

nalimilan reviewed Mar 8, 2018

View reviewed changes

stevengj reviewed Mar 8, 2018

View reviewed changes

fredrikekre force-pushed the fe/nothing branch 2 times, most recently from cfaac3a to 11ad15a Compare March 9, 2018 09:41

fix for find(first|last|next|prev) returning nothing rather than 0:-1

6d7a3cb

fredrikekre force-pushed the fe/nothing branch from 11ad15a to 6d7a3cb Compare March 9, 2018 10:02

fredrikekre requested a review from nalimilan March 9, 2018 10:03

nalimilan approved these changes Mar 9, 2018

View reviewed changes

Do not use isempty

a1a1420

fredrikekre merged commit 5ff9892 into master Mar 9, 2018

fredrikekre deleted the fe/nothing branch March 9, 2018 12:11

nalimilan mentioned this pull request Mar 31, 2018

WIP: add find(first|next|prev|last) variants with nothing sentinel #457

Closed

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for Compat.find* returning nothing from #484 and #…

379a23f

…513

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for Compat.find* from #484 and #513

7335cd9

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for Compat.find* from #484 and #513

381db98

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for Compat.find* from #484 and #513

e7ff254

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add find(first|last|next|prev) methods that return nothing #513

add find(first|last|next|prev) methods that return nothing #513

fredrikekre commented Mar 8, 2018

nalimilan left a comment

nalimilan Mar 8, 2018

nalimilan Mar 8, 2018

fredrikekre Mar 8, 2018 •

edited

Loading

nalimilan Mar 8, 2018

fredrikekre Mar 8, 2018

fredrikekre Mar 8, 2018

fredrikekre Mar 9, 2018

nalimilan Mar 9, 2018

fredrikekre Mar 9, 2018

nalimilan Mar 9, 2018

fredrikekre Mar 9, 2018

stevengj Mar 8, 2018

fredrikekre Mar 8, 2018 •

edited

Loading

stevengj Mar 8, 2018

stevengj Mar 8, 2018 •

edited

Loading

nalimilan Mar 8, 2018

stevengj Mar 8, 2018

nalimilan commented Mar 9, 2018

	Base.findnext(r::Regex, s::AbstractString, idx::Integer) = search(s, r, idx)
	Base.findfirst(r::Regex, s::AbstractString) = search(s, r)
	Base.findnext(c::EqualTo{Char}, s::AbstractString, i::Integer) = search(s, c.x, i)
	Base.findfirst(c::EqualTo{Char}, s::AbstractString) = search(s, c.x)
	Base.findnext(b::EqualTo{<:Union{Int8,UInt8}}, a::Vector{<:Union{Int8,UInt8}}, i::Integer) =
	search(a, b.x, i)
	Base.findfirst(b::EqualTo{<:Union{Int8,UInt8}}, a::Vector{<:Union{Int8,UInt8}}) =
	search(a, b.x)

	Base.findnext(c::OccursIn{<:Union{Tuple{Vararg{Char}},AbstractVector{Char},Set{Char}}},
	s::AbstractString, i::Integer) =
	search(s, c.x, i)
	Base.findfirst(c::OccursIn{<:Union{Tuple{Vararg{Char}},AbstractVector{Char},Set{Char}}},
	s::AbstractString) =
	search(s, c.x)
	Base.findnext(t::AbstractString, s::AbstractString, i::Integer) = search(s, t, i)
	Base.findfirst(t::AbstractString, s::AbstractString) = search(s, t)

	Base.findfirst(delim::EqualTo{UInt8}, buf::Base.IOBuffer) = search(buf, delim.x)

	Base.findprev(c::EqualTo{Char}, s::AbstractString, i::Integer) = rsearch(s, c.x, i)
	Base.findlast(c::EqualTo{Char}, s::AbstractString) = rsearch(s, c.x)
	Base.findprev(b::EqualTo{<:Union{Int8,UInt8}}, a::Vector{<:Union{Int8,UInt8}}, i::Integer) =
	rsearch(a, b.x, i)
	Base.findlast(b::EqualTo{<:Union{Int8,UInt8}}, a::Vector{<:Union{Int8,UInt8}}) =
	rsearch(a, b.x)

	Base.findprev(c::OccursIn{<:Union{Tuple{Vararg{Char}},AbstractVector{Char},Set{Char}}},
	s::AbstractString, i::Integer) = rsearch(s, c.x, i)
	Base.findlast(c::OccursIn{<:Union{Tuple{Vararg{Char}},AbstractVector{Char},Set{Char}}},
	s::AbstractString) = rsearch(s, c.x)
	Base.findprev(t::AbstractString, s::AbstractString, i::Integer) = rsearch(s, t, i)
	Base.findlast(t::AbstractString, s::AbstractString) = rsearch(s, t)

add find(first|last|next|prev) methods that return nothing #513

add find(first|last|next|prev) methods that return nothing #513

Conversation

fredrikekre commented Mar 8, 2018

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Mar 9, 2018

fredrikekre Mar 8, 2018 •

edited

Loading

fredrikekre Mar 8, 2018 •

edited

Loading

stevengj Mar 8, 2018 •

edited

Loading