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

Address breaking change on nightly #226

Merged
merged 6 commits into from
Nov 24, 2017
Merged

Address breaking change on nightly #226

merged 6 commits into from
Nov 24, 2017

Conversation

Nosferican
Copy link
Contributor

Fix to 225

@TotalVerb
Copy link
Collaborator

Let's wait for Compat to support this so this can be done as using Compat.Dates.

@TotalVerb
Copy link
Collaborator

ref JuliaLang/Compat.jl#413

@andreasnoack
Copy link
Member

The new version of Compat is now in METADATA so this can be updated

src/Writer.jl Outdated
@@ -1,5 +1,6 @@
module Writer

using Compat
Copy link
Member

Choose a reason for hiding this comment

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

I think you also need a using Compat.Dates

Copy link
Contributor Author

@Nosferican Nosferican Nov 24, 2017

Choose a reason for hiding this comment

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

@nalimilan is the using Compat.Dates required as well? The documentation in Compat wasn't very clear on it, but let double check.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you need to replace Base.Dates with Compat.Dates everywhere it appeared. So I now realize JuliaData/Missings.jl#56 didn't change anything to the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the case at hand, you just need to replace Dates with Compat.Dates on this line:

Dates.TimeType, Char, Type, AbstractString, Enum, Symbol}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let us check change it and check Travis. After the manual fix both packages compiled so the CI should be cleared.

@stevengj
Copy link
Member

There's no performance difference between using Dates and import Dates (followed by qualified names). As long as there are no namespace collisions, I think using Dates is fine.

@TotalVerb
Copy link
Collaborator

Hi @Nosferican, thank you for the contribution! I went ahead and removed the if isdefined(...) line as mentioned in a review above. I agree that importing/using Dates is not a big deal, so this LGTM once CI passes.

@TotalVerb TotalVerb merged commit 0b191d9 into JuliaIO:master Nov 24, 2017
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