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

Allow x[] for get(x::Nullable) #18766

Closed
wants to merge 6 commits into from

Conversation

TotalVerb
Copy link
Contributor

@TotalVerb TotalVerb commented Oct 2, 2016

No other get function has these semantics, and the x[] is better syntax anyway.

Note that parallel from

dict[key]

to

get(dict, key, default)

versus

nullable[]

to

get(nullable, default)

"""
get(x::Nullable)

Alias for `getindex(x::Nullable)`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I preferred the old docstring in which y was marked as optional.

Copy link
Contributor Author

@TotalVerb TotalVerb Oct 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I found that docstring confusing. (Having both x and y be possibly missing was confusing for me.) Perhaps we can wait for more feedback from others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, it should give direct information instead of redirecting to another docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this?

@nalimilan nalimilan closed this Oct 2, 2016
@nalimilan nalimilan reopened this Oct 2, 2016
@tkelman tkelman added the missing data Base.missing and related functionality label Oct 3, 2016
@davidanthoff
Copy link
Contributor

Not a fan of this syntax, to be honest...

I think if we want a neater syntax for this case, my preferred option would be to wait until #1974 is implemented, and then use x.value for that. The field could just be renamed to _value, and then x.value would call a method that is the same as the current get method. I think that option would be super clear if you read the code, whereas using [] really is quite obscure.

@TotalVerb
Copy link
Contributor Author

I was not expecting this change to be controversial. I suppose a [decision] tag would be warranted, then.


There are two reasons that I prefer this syntax: cleanliness and consistency. Note that [] is already the syntax used by Ref{T} and Array{T,0}, which are both conceptually similar to Nullable{T}. get, on the other hand, is one-of-a-kind. Nothing else in the language uses that name for this operation.

Furthermore, postfix notation is quite natural for accessing values. (That's why most languages use obj.x to access values, which to many reads clearer than x(obj).) In the absence of . overloading, [] is the best postfix operator we have.


Maybe this whole operation should be rethought. Mostly I stumble upon it in conditions like

if isnull(x)
    # blah
else
    foo(get(x))
end

which I personally find very ugly to read and strictly inferior to

if isnull(x)
    # blah
else
    foo(x[])
end

Do many of you see that pattern frequently in your use cases?

but perhaps this entire idiom is quite inferior. It would be nice to have Scala-style pattern matching on Nullable, which would have improved type safety and other general benefits. But that's a separate issue.

@nalimilan
Copy link
Member

but perhaps this entire idiom is quite inferior. It would be nice to have Scala-style pattern matching on Nullable, which would have improved type safety and other general benefits. But that's a separate issue.

#15174

@davidanthoff
Copy link
Contributor

@TotalVerb I completely agree that 1) postfix notation is the way to go here and 2) that these kind of if else blocks are really ugly. My objection is really just an aesthetical one, that I would prefer to wait until the . syntax could be used for this. I think math code often already has lots and lots of brackets, and at least I often find it hard to distinguish the right levels of brackets etc. Adding one more common reason to use brackets would make that kind of code even more busy, whereas something like x.value would be much more pleasing on the eye. Obviously pretty subjective...

@ararslan
Copy link
Member

ararslan commented Oct 7, 2016

I actually disagree that postfix notation is the way to go here--I think it makes it harder to read, plus I fail to see an issue with get(x) or x.value.

@andyferris
Copy link
Member

andyferris commented Oct 11, 2016

I've been thinking hard about an alternative, Swift-inspired syntax for this. Something like

??(x::Nullable) = !x.isnull  # or x.hasvalue
!!(x::Nullable) = x.value

(I'd prefer ? and ! but I'm not sure if that introduces parsing problems with the ternary a ? b : c and potential future lifting of !(::Nullable{Bool})). The idea would be to parse these as unary operators. We could also have a new convenience ternary such as:

x ?? foo(x) : default_value

(where the parser lowers that to ??x ? foo(!!x) : default_value), and which is a lot shorter than the current

if isnull(x)
    default_value
else
    foo(get(x))
end

I'm not sure if this stuff has been discussed before or attempted somewhere?

EDIT: and of course the binary ?? deserves a mention, (x ?? default_value).

@nalimilan
Copy link
Member

@andyferris More syntax for nullables would be nice, but ?? is the null-coalescing operator in other languages such as C# and Swift. So I'd rather use ? like in Swift. Unfortunately, we can't use ! for get (again, like in Swift) since that's a valid character in a Julia identifier.

See also #15174 for another kind of convenience syntax.

get(x::Nullable)

Attempt to access the value of `x`. Throw a `NullException` if the value is not
present.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably mention the x[] syntax here.

@stevengj
Copy link
Member

stevengj commented Oct 11, 2016

Even with dot overloading, it is not at all clear to me that x.value is better than x[]. (And it seems like we may have a long wait before dot overloading happens, if ever.)

👍 for x[], which also mirrors Ref.

@StefanKarpinski
Copy link
Member

Yeah, I have to say that I like x[] too, even though I can see why some find it unappealing.

@StefanKarpinski
Copy link
Member

To elaborate, I view a Nullable, much like a Ref as a scalar (i.e. zero-dimensional) container.

@andyferris
Copy link
Member

To elaborate, I view a Nullable, much like a Ref as a scalar (i.e. zero-dimensional) container.

On that note some of us have been looking for something like Scalar or whatever which can help with some tricky broadcasting problems. Unlike Nullable and Ref such a wrapper wouldn't do anything addition except be an immutable wrapper. (JuliaArrays/StaticArrays.jl#45 (comment))

@TotalVerb
Copy link
Contributor Author

@andyferris I believe that is #18379.

@andyferris
Copy link
Member

Thanks! Perfect

@TotalVerb TotalVerb force-pushed the fw/getindex-nullable branch from 02de8d0 to c550628 Compare October 12, 2016 00:04
@@ -71,7 +71,7 @@ getindex(x::Nullable) = isnull(x) ? throw(NullException()) : x.value
get(x::Nullable)

Attempt to access the value of `x`. Throw a `NullException` if the value is not
present. Equivalent to `getindex(x::Nullable)`.
present. Equivalent to `getindex(x::Nullable)`, which can be spelt `x[]`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since most if not all of the documentation is in American English, you should probably use "spelled" instead of "spelt." I'd say something along the lines of

Equivalent to getindex(x::Nullable), which can alternatively be written as x[].

"""
get(x::Nullable)

Attempt to access the value of `x`. Throw a `NullException` if the value is not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throws?

@davidanthoff
Copy link
Contributor

I should say that I would really welcome anything that makes working with Nullables easier, so please feel free to ignore my aesthetic objections, having thought a bit more about this it is probably better to just get something like that into base.

@davidanthoff
Copy link
Contributor

Looking back over this, I actually really like this, so I'm adding it to DataValues.jl. So, if anyone wants to use this, just wait until queryverse/DataValues.jl#20 is merged. Main use case right now would be in Query.jl, i.e. you should be able to use this in any Query.jl query.

@TotalVerb
Copy link
Contributor Author

If we'd still like this change (which I am still in favor of), chances are that it will be outside Base along with the new Nullable. Thus, closing.

@TotalVerb TotalVerb closed this Jul 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing data Base.missing and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants