Prepare for change from isnull to hasvalue field in Base#150
Prepare for change from isnull to hasvalue field in Base#150davidagold merged 3 commits intomasterfrom
Conversation
| @@ -145,7 +145,7 @@ Let `v, w` be two `Nullable{Float64}` objects. If both `v, w` are non-null, the | |||
| Arguably, the best way to lift existing methods over `Nullable` arguments is to use multiple dispatch. That is, one can very easily extend `f` to handle `Nullable{Float64}` arguments by simply defining an appropriate method: | |||
There was a problem hiding this comment.
I suppose I did write this. I should change this README, since now I believe precisely the opposite. =p
perf/reduce.jl
Outdated
|
|
||
| f(x) = 5 * x | ||
| f{T<:Number}(x::Nullable{T}) = Nullable(5 * x.value, x.isnull) | ||
| f{T<:Number}(x::Nullable{T}) = ifelse(isnull(x), Nullable{typeof(5*x.value)}, Nullable(5 * x.value)) |
There was a problem hiding this comment.
Is the typeof bit necessary?
There was a problem hiding this comment.
Yes, at least if we want to ensure type stability in all cases. But I just realized this is missing (), and the fact that tests haven't failed means that it's not really tested on nulls. Will fix.
src/operators.jl
Outdated
| using Compat: @functorize | ||
|
|
||
| if isdefined(Base, :fieldname) && Base.fieldname(Nullable, 1) == :hasvalue # Julia 0.6 | ||
| Nullable(R, x, hasvalue::Bool) = Nullable{R}(x, hasvalue) |
There was a problem hiding this comment.
Do we need these? I'm loathe to define new methods for Nullable, including constructors, in this package.
There was a problem hiding this comment.
I can call this _Nullable if you prefer to ensure it's not exported.
There was a problem hiding this comment.
Is there any reason we can't just use Nullable{R}(x, hasvalue)?
There was a problem hiding this comment.
Because on 0.4 and 0.5 the second argument is isnull, but on 0.6 it will (probably) be hasvalue.
|
Pesky Should we change the |
Yeah, but we can do that at any point independently of changes in Base. |
0^-1 no longer throws an exception there. Also remove a multiplication to ensure we preserve types instead of promoting to Int64.
In preparation for the change in Julia 0.6, always use isnull() instead of accessing the field directly. Also stop using the Nullable(x, isnull) constructor since the meaning of the second argument is going to change.
The test didn't work.
Current coverage is 84.80% (diff: 93.33%)@@ master #150 diff @@
==========================================
Files 14 14
Lines 860 862 +2
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 736 731 -5
- Misses 124 131 +7
Partials 0 0
|
nalimilan
force-pushed
the
nl/hasvalue
branch
from
59f1e7c to
d12fbab
Compare
3 participants
So that we're ready for JuliaLang/julia#18510. Also include a small fix for
isless.Tests fail on 0.6 because of issues that existed before.