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

Returning native datetime objects for Bucket/Blob time properties. #807

Merged
merged 1 commit into from
Apr 9, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Apr 8, 2015

Note this was almost in #793 but dropped due to RFC3339 questions.

@craigcitro @thobrla can you vet an assumption for me? Will timestamps returned conform to

_GOOGLE_TIMESTAMP_FORMAT = '%Y-%m-%dT%H:%M:%S.%fZ'
# For example
TIME_DELETED = '2014-11-05T20:34:37.000Z'

I ask because '2014-11-05T20:34:37Z' is also totally valid according to RFC3339 and I'm wondering if we can get away with using datetime alone (and not having to use the strict-rfc3339 library for the full generality of RFC3339).

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 8, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9f7d6fc on dhermes:use-native-datetime into * on GoogleCloudPlatform:master*.

@thobrla
Copy link

thobrla commented Apr 8, 2015

I think this is true, but it is an implementation detail so it could break in the future; the discovery document only claims this is in RFC3339 format.

That being said, apitools just uses protorpc, which uses datetime exclusively. @craigcitro may be able to comment further; looking at the code we treat the timestamps as DateTimeFields, https://github.com/google/protorpc/blob/master/protorpc/message_types.py#L52, but these are in millisecond format, not RFC3339 and I can't find where it does the string conversion.

BLOB_NAME = 'blob-name'
connection = _Connection()
bucket = _Bucket(connection)
TIME_DELETED = '2014-11-05T20:34:37Z'
TIME_DELETED = '2014-11-05T20:34:37.000Z'
TIMESTAMP = datetime.datetime(2014, 11, 5, 20, 34, 37)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 8, 2015

@thobrla The conversion happens in apitools

@thobrla
Copy link

thobrla commented Apr 8, 2015

Ah; the responsible code is actually in protorpc, and that handles both kinds of string formats (using text search and strptime). That seems like a reasonable approach.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 8, 2015

Yes, I'm cool doing that (only because we will end up requiring protorpc via apitools anyhow).

@craigcitro
Copy link
Contributor

one more heads-up about timestamps: some newer APIs (eg pubsub) may return you timestamps with even more precision. i.e. down to the nanosecond. (see source here)

@dhermes
Copy link
Contributor Author

dhermes commented Apr 9, 2015

@craigcitro WAT? datetime doesn't support nanoseconds, so I suppose I should hold off. Oh Google, le sigh.

@craigcitro
Copy link
Contributor

well, your fix is still good for storage. we can cross the next bridge when we come to it.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 9, 2015

@craigcitro You mean this PR is good or the suggestion to use protorpc.util.decode_datetime?

As it were, I can confirm that the timezone support and the option of not having milliseconds (the only extra features) are not necessary.

From the discovery type documentation:

Type value Format value Meaning
string date-time An RFC3339 timestamp in UTC time. This is formatted as YYYY-MM-DDThh:mm:ss.fffZ. The milliseconds portion (".fff") is optional. Defined in the JSON Schema spec.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 9, 2015

@tseaver I made an update to the unit tests and also made a slight tweak to the use of mtime in Blob.download_to_filename (was some repeated code).

PTAL

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2645e1d on dhermes:use-native-datetime into * on GoogleCloudPlatform:master*.

@tseaver
Copy link
Contributor

tseaver commented Apr 9, 2015

LGTM

dhermes added a commit that referenced this pull request Apr 9, 2015
Returning native datetime objects for Bucket/Blob time properties.
@dhermes dhermes merged commit 3f6ee7b into googleapis:master Apr 9, 2015
@dhermes dhermes deleted the use-native-datetime branch April 9, 2015 17:56
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants