-
Notifications
You must be signed in to change notification settings - Fork 21
Added ability to convert from Arrays of Nullables #99
Added ability to convert from Arrays of Nullables #99
Conversation
Hmm... I only checked Julia 0.4. Did I cause the nightly build test to break, or was it broken already? |
|
||
#----- Conversion from Array of Nullables #1 -----------------------------------# | ||
# Take an Array of Nullable objects and convert it into a NullableArray | ||
function Base.convert{S,T,N}(::Type{NullableArray{S,N}},in::Array{Nullable{T},N}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add spaces after commas as in the rest of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, and you don't need the T
here and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the code review, @nalimilan. How will it work without T
? For example, we don't have Array{Nullable{Int,N}} <: Array{Nullable}
, and I didn't want this function to capture Array
s that were filled with something non-Nullable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant in::Array{Nullable,N}
, but now I see that you need to keep T
in {S,T,N}
anyway, so you can as well keep it.
Thanks. The Travis failure doesn't look related, but I guess it should be fixed anyway. |
out | ||
end | ||
|
||
#----- Conversion from Array of Nullables #1 -----------------------------------# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate of the method above?
I would like to follow best practices and not merge in the presence of even seemingly unrelated failures. We need to do a pretty thorough review of how Base Julia is impacting the present package. If people want to help me sleuth through this, I'd welcome it. I have finals next week and won't be able to sit down and seriously examine these issues till after then. EDIT: I will mention I appreciate this PR and that I don't like holding up merging it on account of what are probably unrelated failures. |
Actually, @nalimilan @johnmyleswhite do you have any opinions about merging this once it's ready despite the state of this package on v0.5? I can put something in the README to emphasize that we don't claim to support 0.5 yet. |
As long as the tests pass on 0.4, it should be fine (unless the tests pass on 0.5 without this PR of course). Though there are still a few unadressed points. |
Yes, there are. But once those are addressed, I think I'll be comfortable merging. The breakage points are not new to this PR. Thanks for weighing in, @nalimilan . |
Agree with Milan that you should focus on making sure tests pass on 0.4. Targeting 0.5 gives you a moving target. |
Indeed. Okay, PRs need pass tests only on 0.4. Thank you for your thoughts, John. |
@andyferris Would you mind rebasing your PR? We were quite close from merging when the discussion stopped. |
Sorry, yes I have been distracted by many things (moving house, etc) and this one went on the back-burner. I'll try have a look tomorrow! |
f00c994
to
406851b
Compare
Changes Unknown when pulling 406851b on andyferris:pull-request/84efecbd into * on JuliaStats:master*. |
406851b
to
af609b1
Compare
OK, rebased and removed an duplicated function. Tests pending, but could be good to go? |
Changes Unknown when pulling af609b1 on andyferris:pull-request/84efecbd into * on JuliaStats:master*. |
Thanks! Looks good to me, if @davidagold wants to merge. Though you should probably squash the last commit. |
Yes, would you please squash those into one commit? I'd greatly appreciate that =) Otherwise, this looks good to go. |
Thanks, @andyferris! I've squashed them directly using the new GitHub support for that. |
These methods add the ability to create a
NullableArray
from data that is in anArray{Nullable{T},N}
. Tests added.See #98