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

Use Base.Missing on Julia 0.7 #68

Merged
merged 1 commit into from
Dec 12, 2017
Merged

Use Base.Missing on Julia 0.7 #68

merged 1 commit into from
Dec 12, 2017

Conversation

nalimilan
Copy link
Member

Adapt to JuliaLang/julia#24653. Tests still fail on 0.7 due to a promote_type bug in Julia, but most features work.

using Compat

export allowmissing, disallowmissing, ismissing, missing, missings,
Missing, MissingException, levels, skipmissing

"""
Missing
if VERSION < v"0.7.0-DEV.2762"
Copy link
Member Author

Choose a reason for hiding this comment

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

It's kind of weird, but if I define the types in the same if block as the rest I get type definition not allowed inside a local scope errors.

Copy link
Member

Choose a reason for hiding this comment

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

That's really bizarre. What if you put an @static annotation on the if?

Copy link
Member Author

@nalimilan nalimilan Dec 8, 2017

Choose a reason for hiding this comment

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

Same. AFAIK @static has no effect at the top level. I've found this issue, but it's pretty old and apparently closed: JuliaLang/julia#10555

EDIT: actually it's not closed, it's just a dup. Probably related to the presence of a for loop.

Copy link
Member

Choose a reason for hiding this comment

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

@static is used at the top level in Compat.jl: https://github.com/JuliaLang/Compat.jl/blob/master/src/Compat.jl

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I can swear that I've read a comment by Jameson saying that has no effect.

Copy link
Member

Choose a reason for hiding this comment

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

I used it to hide old deprecated syntax from 0.7: https://github.com/JuliaLang/Compat.jl/pull/419/files

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that since the condition is evaluated at parse time, it gives an opportunity to trick the parser into ignoring things doesn't particularly like.

Copy link
Contributor

@cjprybol cjprybol left a comment

Choose a reason for hiding this comment

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

Now that Missing is in base, is this the last set of changes we want to make here? Should all future changes be made in base (and copied here for the duration of 0.6 support)?

@nalimilan
Copy link
Member Author

I guess so. However it's not clear whether we'll be able to move everything to Base. For example I'm not sure missings, allowmissing and disallowmissing! will be accepted. We'll see...

@nalimilan nalimilan merged commit 392b205 into master Dec 12, 2017
@nalimilan nalimilan deleted the nl/0.7 branch December 12, 2017 09:29
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