Conversation
| @test !isempty(A) | ||
| A = empty!(A) | ||
| @test isempty(A) | ||
| end |
There was a problem hiding this comment.
Would be good to test that elements are compared with ===, e.g. using an IdSet{Vector{Int}}.
There was a problem hiding this comment.
Maybe something like
julia> s = Base.IdSet{Vector{Int}}([a, b])
Base.IdSet{Array{Int64,1}} with 2 elements:
[1]
[2]
julia> a ∈ s
true
julia> c = [1]
1-element Array{Int64,1}:
1
julia> c ∈ s
falseThere was a problem hiding this comment.
Would probably be good to also add something that is == a, but not === a to test that it's not just a regular Dict.
There was a problem hiding this comment.
To make such a test, I guess one could e.g. use Vector{Int} keys:
julia> dict = IdDict([0] => 1, [0] => 2)
IdDict{Array{Int64,1},Int64} with 2 entries:
[0] => 1
[0] => 2There was a problem hiding this comment.
That said, this PR looks like a nice improvement, so even without that, it'd be nice to merge it (and improve it later?) Depending on whether @kshyatt still has any interest on "finishing" this or not, it'd be a shame to let this go to waste?
|
pinging again gently for a second review :) |
|
I'm still interested :). When I get a free moment I'll update it.
… On Jan 2, 2021, at 11:06 AM, Max Horn ***@***.***> wrote:
@fingolfin commented on this pull request.
In test/sets.jl:
> ***@***.*** "IdSet" begin
+ A = Base.IdSet{Int}(1:4)
+ @test !isempty(A)
+ B = copy(A)
+ @test A ⊆ B
+ @test B ⊆ A
+ A = filter!(isodd, A)
+ @test A ⊆ B
+ @test !(B ⊆ A)
+ @test !isempty(A)
+ a = pop!(A, 1)
+ @test a == 1
+ @test !isempty(A)
+ A = empty!(A)
+ @test isempty(A)
+end
That said, this PR looks like a nice improvement, so even without that, it'd be nice to merge it (and improve it later?) Depending on whether @kshyatt still has any interest on "finishing" this or not, it'd be a shame to let this go to waste?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Anything to be done here? Does this still need review or can we just merge it? |
|
Updated this PR. I think we can just merge it as is if it passes tests and make further improvements later. |
|
Got the ok from @StefanKarpinski to merge |
Appears to be missing tests.