Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

WIP: support NullableArrays conversions if #undef values are null #79

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

davidagold
Copy link
Contributor

This PR is a pre-emptive fix for the issue described in JuliaStats/DataArrays.jl#107. With it, the following works:

julia> B = Array(Any, 8);

julia> X = NullableArray(B, fill(true, 8));

julia> X[2:8] = collect(2:8);

julia> convert(NullableArray{Float64}, X)
8-element NullableArray{Float64,1}:
 #NULL  
     2.0
     3.0
     4.0
     5.0
     6.0
     7.0
     8.0

HOWEVER, something strange is going on with the tests. Prior to the present PR, we only had a

Base.convert{S, T, N}(::Type{NullableArray{S, N}}, X::NullableArray{T, N})

method. I added a

Base.convert{S, T, N}(::Type{NullableArray{S}}, X::NullableArray{T, N})

method, since the N parameter in the first argument is redundant. I coded the new behavior into this second method, and then tried defining the first method in terms of the second. For some reason, this causes tests in test/map.jl and sometimes test/subarray.jl to fail -- even though the return value of the new methods are equal under isequals to the return values of the old methods. Leaving the first method as is but including the second, new method seems to pass just fine.

I'm going to continue trying to figure out what's wrong -- it may be a problem with the tests themselves. But this has really got me scratching my head, and if anybody has a lead I would appreciate it.

@codecov-io
Copy link

Current coverage is 90.55%

Merging #79 into master will decrease coverage by -0.06% as of ca1df26

@@            master     #79   diff @@
======================================
  Files           12      12       
  Stmts          586     593     +7
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            531     537     +6
  Partial          0       0       
- Missed          55      56     +1

Review entire Coverage Diff as of ca1df26

Powered by Codecov. Updated on successful CI builds.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants