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

Addressing Breaking Change to Dates #56

Merged
merged 2 commits into from Nov 24, 2017
Merged

Addressing Breaking Change to Dates #56

merged 2 commits into from Nov 24, 2017

Conversation

Nosferican
Copy link
Contributor

@Nosferican Nosferican commented Nov 22, 2017

Addressing 0c0d6cc
If Nulls is getting a version for Julia 0.7 it would need a similar fix (until Compat?).

src/Missings.jl Outdated
@@ -2,6 +2,9 @@ __precompile__(true)
module Missings

import Base: *, <, ==, !=, <=, !, +, -, ^, /, &, |, xor
if VERSION >= v"0.7.0-DEV.2575"
import Dates
Copy link
Member

Choose a reason for hiding this comment

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

I guess using should be enough, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, but import is safer since Base.Dates used to export only certain names. I believe they should be the same now that it moved to stdlib.

Copy link
Member

Choose a reason for hiding this comment

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

No, it's not safer, as it allows overriding functions in Dates, which is not what we want to do AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on the style. using has no overriding allowed, but floods the namespace (especially with packages that export many functions and structs) and can cause issues if multiple packages have the same functions. import allows to override (though you still have to do it explicitly), but keeps the namespace clear. There is also a warning if any package overrides a method. I always use the module prefix (except with Base) since I saw it in an old Julia blog. Either one would do and probably Compat will probably check for the optimal solution.

Copy link
Member

Choose a reason for hiding this comment

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

using also allows specifying which objects you want to use.

@nalimilan
Copy link
Member

Thanks. I think we'd better wait for Compat to support this, though.

@omus
Copy link
Member

omus commented Nov 22, 2017

JuliaLang/Compat.jl#413

@codecov-io
Copy link

codecov-io commented Nov 22, 2017

Codecov Report

Merging #56 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #56   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files           1        1           
  Lines         151      151           
=======================================
  Hits          142      142           
  Misses          9        9
Impacted Files Coverage Δ
src/Missings.jl 94.03% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c08d4bb...6593291. Read the comment docs.

Addressing [0c0d6cc](JuliaLang/julia@0c0d6cc)
If Nulls is getting a version for Julia 0.7 it would need a similar fix (until Compat?).
@nalimilan nalimilan merged commit 130a2f3 into JuliaData:master Nov 24, 2017
@nalimilan
Copy link
Member

Thanks!

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