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.

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

additional convert methods for Nullable #9351

Merged
merged 1 commit into from
Dec 15, 2014
Merged

additional convert methods for Nullable #9351

merged 1 commit into from
Dec 15, 2014

Conversation

amitmurthy
Copy link
Contributor

Allows for :

julia> type Bar
         a::Nullable{Int}
         b::Nullable{Bool}

         Bar() = new(nothing, nothing)
       end

julia> bar=Bar()
Bar(Nullable{Int64}(),Nullable{Bool}())

julia> bar.a=1
Nullable(1)

julia> bar.a=nothing
Nullable{Int64}()

EDIT: The PR has been updated to only support assignment of a non-null value to a Nullable field. See discussion below.

@amitmurthy
Copy link
Contributor Author

cc @johnmyleswhite

@JeffBezanson
Copy link
Member

I think the first definition is fine, but a non-null value should not be silently converted to null. We should use a constant Nullable{Union()}() for that.

@nalimilan
Copy link
Member

The bar.a = nothing part reminds me of this discussion: #8423 (comment)

I think your changes are justified disregarding that debate (one should be able to convert nothing into a Nullable), but whether bar.a = nothing should be considered as idiomatic and encouraged may be more controversial.

@ivarne
Copy link
Member

ivarne commented Dec 14, 2014

Before merging we should probably also add some tests for the new methods.

@amitmurthy
Copy link
Contributor Author

@ivarne sure.

Since NULL never had consensus in #8423 (comment), how about defining const EMPTY = Nullable{Union()}()

Also, another issue of minor confusion is that since the documentation says that Nullable is conceptually a container of 0 or 1 elements of type T, one would expect to be able to empty a Nullable object, i.e. with something like a empty!(x::Nullable). But this is meaningless since Nullable is both an immutable and there is no way to set x.value to undefined.

@johnmyleswhite
Copy link
Member

I don't think there's a contradiction there: you can have immutable containers. Julia doesn't have enough of them yet, but they are useful. Nullable happens to be one of the first examples of a Julian immutable container, aside from tuples.

Quoting what I said on the mailing list:

Having hit this issue a lot, I'm very sympathetic to the idea of adding those convert methods. I'm not totally convinced since I do want people to know whether they're seeing Nullable objects or "direct" objects without a Nullable wrapper, but I'm 99% convinced that adding convert{T}(::Type{Nullable{T}}, x::T) = Nullable{T}(x) is a good idea.

I'm more skeptical about inserting Void to mean Nullable, since I worry that people will start to conflate Void with Nullable if we start making them quasi-interchangeable.

@johnmyleswhite
Copy link
Member

I would be less unhappy supporting const NULL = Nullable{Union()}() if we started raising type-instability warnings by default. I'm just afraid of people writing functions like:

function mean(x::Array{Nullable{Float64}})
  s, n = 0.0, length(x)
  for i in 1:n
    if isnull(x[i])
      return NULL
    else
      s += get(x[i])
    end
  end
  return Nullable(s / n)
end

@JeffBezanson
Copy link
Member

Union types have a really bad rap, but inference of get on Union(Nullable{Union()},Nullable{Float64})) will give Float64.

@amitmurthy
Copy link
Contributor Author

OK. I have changed this PR to only include the first method. Will merge this is a short while since we more or less have consensus on this.

Will open a new issue to generate consensus on a less verbose way of making a Nullable field null.

amitmurthy added a commit that referenced this pull request Dec 15, 2014
additional convert method for Nullable
@amitmurthy amitmurthy merged commit cf8e53d into JuliaLang:master Dec 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants