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

docs: require timezone aware datetimes #6581

Merged
merged 2 commits into from
Aug 17, 2021
Merged

Conversation

busunkim96
Copy link
Contributor

@busunkim96 busunkim96 commented Aug 16, 2021

Follow up to discussion in googleapis/python-tasks#147 (comment).

If this is an OK blanket recommendation to make, I will follow up with a PR to adjust all the calls to datetime.datetime in this repository.

For supporting documentation, see warnings for these two methods:

@busunkim96 busunkim96 requested a review from a team as a code owner August 16, 2021 23:33
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Aug 16, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 16, 2021
@leahecole
Copy link
Collaborator

Now I'm curious about what kinds of mayhem not doing this can cause 😁

@leahecole leahecole added the automerge Merge the pull request once unit tests and other checks pass. label Aug 17, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 732d5ef into master Aug 17, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the datetime-note branch August 17, 2021 17:18
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 17, 2021
@tswast
Copy link
Contributor

tswast commented Aug 17, 2021

There's a DATETIME data type in BigQuery that's specifically not timezone-aware, but I still think this makes sense as general advice when not dealing with a data type where it doesn't make sense.

https://stackoverflow.com/a/47724366/101923


Always create timezone aware datetime objects. For libraries that use protobuf,
omitting the timezone may lead to unexpected behavior when the datetime
is converted to a protobuf tiemstamp.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is converted to a protobuf tiemstamp.
is converted to a protobuf timestamp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants