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

Export coalesce #72

Merged
merged 2 commits into from
Jan 18, 2018
Merged

Export coalesce #72

merged 2 commits into from
Jan 18, 2018

Conversation

ararslan
Copy link
Member

Fixes #71. Not sure why this wasn't exported before.

@ararslan ararslan requested a review from nalimilan January 18, 2018 00:39
src/Missings.jl Outdated
@@ -30,7 +30,7 @@ end
import Base: ==, !=, <, *, <=, !, +, -, ^, /, &, |, xor

if VERSION >= v"0.7.0-DEV.2762"
using Base: ismissing, missing, Missing, MissingException
using Base: coalesce, ismissing, missing, Missing, MissingException
Copy link
Member

Choose a reason for hiding this comment

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

IIRC coalesce has been added later, when I removed Nullable, so it should be under a different version check.

@quinnj
Copy link
Member

quinnj commented Jan 18, 2018

I had a fix for this locally along w/ some other 0.7 compat. Any objections to merging?

@quinnj quinnj mentioned this pull request Jan 18, 2018

# Deprecations
@deprecate skip(itr) skipmissing(itr) false
Compat.coalesce(x::Missing) = missing
Copy link
Member

Choose a reason for hiding this comment

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

This should only be defined before Base includes coalesce, or it's going to overwrite the equivalent Base method.

Copy link
Member

Choose a reason for hiding this comment

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

That assumes that Compat already defines coalesce(x::Missing), which it currently doesn't (it's commented out). It really boils down to whether the actual compat definition is in Missings or Compat.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but I was speaking about a different issue, which is that recent Julia 0.7 versions define that method, and that Compat.coalesce = Base.coalesce there.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so? I don't see any overwrite happening on latest master?

Copy link
Member Author

Choose a reason for hiding this comment

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

Easy enough to just wrap this in is !isdefined(Base, :coalesce) regardless.

@quinnj quinnj merged commit 528e5dc into master Jan 18, 2018
@quinnj quinnj deleted the aa/export branch January 18, 2018 21:20
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.

4 participants