-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(datastore): support civil package types save #3202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make changes to load as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's make sure to have a user test with a pseudo-version before tagging a release. Please wait for @tritone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @crwilcox @BenWhitehead could you please take a look before we merge?
What is the experience for someone who doesn't have civil? |
No changes. This should only change behavior for people who are using |
@tbpg so there is no chance of panics or anything from a missing import? |
@crwilcox, no. I don't think there is a way to cause a panic from a missing import in Go. If something is used, it is imported, otherwise there is a compile-time error. |
Thank you for clearing things up @tbpg. I had missed that the package already depended on civil which is why I was a bit confused how this worked. lgtm. |
case typeOfCivilDate: | ||
date := civil.DateOf(pValue.(time.Time)) | ||
v.Set(reflect.ValueOf(date)) | ||
case typeOfCivilDateTime: | ||
dateTime := civil.DateTimeOf(pValue.(time.Time)) | ||
v.Set(reflect.ValueOf(dateTime)) | ||
case typeOfCivilTime: | ||
timeVal := civil.TimeOf(pValue.(time.Time)) | ||
v.Set(reflect.ValueOf(timeVal)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @IlyaFaer for working on this!
I'm following up via #3089.
There's an issue loading civil dates and times on a machine when the timezone is set to anything other than UTC. Time properties are loaded in a machine's local location (see #916) and therefore need to be converted to UTC before using them to create civil values.
I think this is the adjustment that's needed:
case typeOfCivilDate:
date := civil.DateOf(pValue.(time.Time).In(time.UTC))
v.Set(reflect.ValueOf(date))
case typeOfCivilDateTime:
dateTime := civil.DateTimeOf(pValue.(time.Time).In(time.UTC))
v.Set(reflect.ValueOf(dateTime))
case typeOfCivilTime:
timeVal := civil.TimeOf(pValue.(time.Time).In(time.UTC))
v.Set(reflect.ValueOf(timeVal))
@dnerdy Would you please open an issue for this, thanks for finding this! |
Support saving properties of
civil
package types:Date
,Time
andDateTime
.Closes #3089