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 DateTime constructor for Base.Libc.Tmstruct #31575

Merged
merged 4 commits into from
Apr 18, 2019

Conversation

eulerkochy
Copy link
Contributor

Fixes #31524

@KristofferC KristofferC added the needs tests Unit tests are required for this change label Apr 1, 2019
@JeffBezanson JeffBezanson removed the needs tests Unit tests are required for this change label Apr 1, 2019
@eulerkochy
Copy link
Contributor Author

Could this be reviewed before feature freeze? Can this be considered as a feature @StefanKarpinski ?

@StefanKarpinski StefanKarpinski requested a review from quinnj April 2, 2019 14:47
@StefanKarpinski
Copy link
Member

This seems sane to me. @quinnj: can you do a quick sanity check and merge if it's good?

@fredrikekre fredrikekre added dates Dates, times, and the Dates stdlib module stdlib Julia's standard library labels Apr 2, 2019
stdlib/Dates/src/types.jl Outdated Show resolved Hide resolved
@eulerkochy
Copy link
Contributor Author

@quinnj is it alright now?

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

So here's what the docs say for the meaning of TmStruct fields:

Member Type Meaning Range
tm_sec int seconds after the minute 0-61*
tm_min int minutes after the hour 0-59
tm_hour int hours since midnight 0-23
tm_mday int day of the month 1-31
tm_mon int months since January 0-11
tm_year int years since 1900  
tm_wday int days since Sunday 0-6
tm_yday int days since January 1 0-365
tm_isdst int Daylight Saving Time flag

So we'll need to adjust the DateTime and Date constructors to account for the TmStruct month being 0-based (0 - 11), and the year being "years since 1900". So if we can make those changes and then add some tests making sure it's all being converted correctly, I think this is looking good.

@eulerkochy
Copy link
Contributor Author

@quinnj thanks for the type definition, I've corrected the constructor definition, and have added tests.

@eulerkochy
Copy link
Contributor Author

@StefanKarpinski @quinnj when is the feature freeze?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Apr 4, 2019

Today. Might get a bit delayed on account of ongoing CI flakiness.

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

I think this looks good now!

@eulerkochy
Copy link
Contributor Author

Bump

@quinnj quinnj merged commit 36586c6 into JuliaLang:master Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ability to convert TmStruct to DateTime?
6 participants