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

Add basic support for datetime types with timezones #21

Merged
merged 1 commit into from
Sep 5, 2020

Conversation

athre0z
Copy link
Member

@athre0z athre0z commented Sep 5, 2020

This adds support for selecting from and inserting into columns that have a timezone set. Because Julia's DateTime type isn't timezone aware, we simply ignore the timezone and leave handling and conversion to the user. While this is unfortunate, as previously mentioned in this issue, the TimeZones.jl library has some performance issues in its current form and just isn't a real option until this is resolved.

Do You mean DateTime(timestamp) and DateTime64(precision, timestamp)?

Yes -- at least if you meant timezone where you wrote timestamp.

@athre0z athre0z requested a review from waralex September 5, 2020 14:32
Copy link
Collaborator

@waralex waralex left a comment

Choose a reason for hiding this comment

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

Everything's fine. When it will possible to implement work with the time zone without running costs it may make sense to divide the functions into 2 overloads

@athre0z athre0z merged commit 62bb1ee into master Sep 5, 2020
@athre0z athre0z deleted the feature/timezones branch September 5, 2020 20:41
@athre0z
Copy link
Member Author

athre0z commented Sep 5, 2020

Alright then -- I guess we'd be ready for a new release then, unless you have anything that you'd like to get done before?

Unless you have anything else, I'd like to offer you to also put your name here and, while you're editing this file anyway, bump the version to 0.2.0. :) Feel free to directly push this to master without going through a branch first btw.

(I said that we're currently at 0.2 in previous messaging but looking again, this turned out to be wrong)

@waralex
Copy link
Collaborator

waralex commented Sep 5, 2020

I'd like to offer you

Done. And thank you very much for the offer, I am pleased.

One more thing

DataFrames = ">= 0.19"
ProgressMeter = ">= 1.1"

It seems to me that this will violate the conditions for auto-merging the request to the Registrator. Anyway in my of components for Dash this led to this warning:

The following dependencies do not have a compat entry that has an upper bound: DashBase. You may find CompatHelper helpful for keeping your compat entries up-to-date.Note: If your package works for the current version x.y.z of a dependency foo, then a compat entry foo = x.y.z implies a compatibility upper bound for packages following semver. You can additionally include earlier versions your package is compatible with. See https://julialang.github.io/Pkg.jl/v1/compatibility/ for details.

@athre0z
Copy link
Member Author

athre0z commented Sep 5, 2020

That's good to know. I fixed the versions and tried to ping the registration bot, but it looks like it didn't like the repo migration and now I'll have to make a PR ...

@waralex
Copy link
Collaborator

waralex commented Sep 5, 2020

but it looks like it didn't like the repo migration and now I'll have to make a PR

I didn't manage to send requests from the plotly repositories for registration via the bot, but they were sent via https://juliahub.com/ui/index.html without any problems

@athre0z
Copy link
Member Author

athre0z commented Sep 5, 2020

Hmm, interesting, but I don't think that would have worked for changing the URL of a package that is already registered. Looking at the PRs in the General repo, a lot of other people are also changing their URLs via manual PRs. I created one just now:

JuliaRegistries/General#20909

@waralex
Copy link
Collaborator

waralex commented Sep 5, 2020

Okay, thanks. Will wait :)

@waralex
Copy link
Collaborator

waralex commented Sep 5, 2020

And one more thing I remembered. We should not forget to write in the documentation that we do not take into account the time zone when reading. I would prefer that you do this, because with my command of English, it may not be very clear.

This doesn't cancel the release, we just need to remember to do it

@athre0z
Copy link
Member Author

athre0z commented Sep 6, 2020

Yes, good point. Do you think putting it in the "Limitations" section in the doc is sufficient or would you want it somewhere else as well?

@waralex
Copy link
Collaborator

waralex commented Sep 7, 2020

Do you think putting it in the "Limitations" section in the doc is sufficient

Yes, I think that will be enough

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.

2 participants