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

datastore: support civil.Date for properties #3089

Closed
benjaminjkraft opened this issue Oct 26, 2020 · 6 comments · Fixed by #3202
Closed

datastore: support civil.Date for properties #3089

benjaminjkraft opened this issue Oct 26, 2020 · 6 comments · Fixed by #3202
Assignees
Labels
api: datastore Issues related to the Datastore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@benjaminjkraft
Copy link
Contributor

Is your feature request related to a problem? Please describe.
We sometimes store dates-without-location (in the sense of the civil package) in datastore. Sadly, that means we have to handle them, temporarily at least, as time.Time. This is sort of a foot-gun for all the usual reasons (described, for example, in e.g. golang/go#19700).

Describe the solution you'd like
We could have a struct-field of type civil.Date; that way we never have to have a time.Time-representing-a-civil-date floating around. Ideally this would be compatible with how the libraries in other languages, some of which have a date-property, do things.

Describe alternatives you've considered
We could write a PropertyLoadSaver for each such model, and specially handle the field. That's definitely possible but it's some extra work for each such model.

@benjaminjkraft benjaminjkraft added the triage me I really want to be triaged. label Oct 26, 2020
@product-auto-label product-auto-label bot added the api: datastore Issues related to the Datastore API. label Oct 26, 2020
@tritone tritone added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Oct 30, 2020
@tritone
Copy link
Contributor

tritone commented Nov 2, 2020

This seems like a good thing to add support for. @IlyaFaer could you take this one? The civil package is cloud.google.com/go/civil .

@IlyaFaer
Copy link

IlyaFaer commented Nov 3, 2020

@tritone, sure, I'll take a look.

@tritone
Copy link
Contributor

tritone commented Dec 2, 2020

We've merged this change. @benjaminjkraft would it be possible for you to test this for your purposes at HEAD before we cut a release?

@benjaminjkraft
Copy link
Contributor Author

benjaminjkraft commented Dec 2, 2020

Sure! Let me follow up with @MiguelCastillo and/or @dnerdy but I think we should be able to pull this in and get some testing soon. Thanks much for implementing this, it will definitely simplify some things for us!

@dnerdy
Copy link

dnerdy commented Dec 3, 2020

@tritone It looks like there's a bug. I've left a comment with details on #3202.

@dnerdy
Copy link

dnerdy commented Dec 3, 2020

I've opened a separate issue for the bug: #3360.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants