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

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

Merged
merged 2 commits into from
Mar 9, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d7a3cb
fix for find(first|last|next|prev) returning nothing rather than 0:-1
fredrikekre Mar 9, 2018
a1a1420
Do not use isempty
nalimilan Mar 9, 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
4 changes: 3 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,8 @@ Currently, the `@compat` macro supports the following syntaxes:
sometimes combined with `equalto` or `occursin` ([#24673]).

* `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]).
return `nothing` when no match is found (rather than `0` or `0:-1`)
as on Julia 0.7 ([#24673], [#26149]).

* `findin(a, b)` is now `findall(occursin(b), a)` ([#24673]).

Expand Down Expand Up @@ -586,4 +587,5 @@ includes this fix. Find the minimum version from there.
[#25990]: https://github.com/JuliaLang/julia/issues/25990
[#26069]: https://github.com/JuliaLang/julia/issues/26069
[#26089]: https://github.com/JuliaLang/julia/issues/26089
[#26149]: https://github.com/JuliaLang/julia/issues/26149
[#26156]: https://github.com/JuliaLang/julia/issues/26156
1 change: 1 addition & 0 deletions src/Compat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1498,6 +1498,7 @@ else
export occursin

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

Choose a reason for hiding this comment

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

Maybe use isempty just in case?

Copy link
Member

@stevengj stevengj Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Fine, no big deal either way.

zero2nothing(x) = x

findnext(xs...) = zero2nothing(Base.findnext(xs...))
Expand Down
25 changes: 13 additions & 12 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1378,18 +1378,19 @@ 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),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@fredrikekre fredrikekre Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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 2. already work?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough.

(findlast, findprev, 2),
(Compat.findfirst, Compat.findnext, 1),
(Compat.findlast, Compat.findprev, 2))
@test f2("a", "ba", i) == f1("a", "ba") == 2:2
@test f2("z", "ba", i) == f1("z", "ba") == 0:-1
end
for (f1, f2, i) in ((findfirst, findnext, 1),
(Compat.findfirst, Compat.findnext, 1))
@test f2(r"a", "ba", 1) == f1(r"a", "ba") == 2:2
@test f2(r"z", "ba", 1) == f1(r"z", "ba") == 0:-1
end
@test findnext("a", "ba", 1) == findfirst("a", "ba") == 2:2
@test findnext("z", "ba", 1) == findfirst("z", "ba") == (VERSION < v"0.7.0-DEV.4480" ? (0:-1) : nothing)
@test findprev("a", "ba", 2) == findlast("a", "ba") == 2:2
@test findprev("z", "ba", 2) == findlast("z", "ba") == (VERSION < v"0.7.0-DEV.4480" ? (0:-1) : nothing)
@test Compat.findnext("a", "ba", 1) == Compat.findfirst("a", "ba") == 2:2
@test Compat.findnext("z", "ba", 1) == Compat.findfirst("z", "ba") == nothing
@test Compat.findprev("a", "ba", 2) == Compat.findlast("a", "ba") == 2:2
@test Compat.findprev("z", "ba", 2) == Compat.findlast("z", "ba") == nothing

@test findnext(r"a", "ba", 1) == findfirst(r"a", "ba") == 2:2
@test findnext(r"z", "ba", 1) == findfirst(r"z", "ba") == (VERSION < v"0.7.0-DEV.4480" ? (0:-1) : nothing)
@test Compat.findnext(r"a", "ba", 1) == Compat.findfirst(r"a", "ba") == 2:2
@test Compat.findnext(r"z", "ba", 1) == Compat.findfirst(r"z", "ba") == nothing

@test Compat.findfirst(equalto(UInt8(0)), IOBuffer(UInt8[1, 0])) == 2
@test Compat.findfirst(equalto(UInt8(9)), IOBuffer(UInt8[1, 0])) == nothing
Expand Down