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 'Client.list_entries'. #1569

Merged
merged 1 commit into from
Mar 11, 2016
Merged

Add 'Client.list_entries'. #1569

merged 1 commit into from
Mar 11, 2016

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Mar 3, 2016

No description provided.

@tseaver tseaver added the api: logging Issues related to the Cloud Logging API. label Mar 3, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 3, 2016
@tseaver
Copy link
Contributor Author

tseaver commented Mar 7, 2016

@dhermes PTAL. I will rebase #1574 and assign to you after this one lands.

dt_str, _RFC3339_MICROS).replace(tzinfo=UTC)
except ValueError: # maybe nanosecond resoultion
prefix, suffix = dt_str.split('.')
fraction, zulu = suffix[:-1], suffix[-1]

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Mar 8, 2016

Sorry I've gotta stop midway through the review, have to go teach

@tseaver
Copy link
Contributor Author

tseaver commented Mar 9, 2016

@dhermes this one should be good to go.

:param resource: one entry resource from API response

:type loggers: dict or None
:param loggers: A mapping of loggerr fullnames -> loggers. If not

This comment was marked as spam.

This comment was marked as spam.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 10, 2016

@dhermes In 4f38261, _rfc3339_to_datetime uses a regex to identify/parse the nanosecond-precision timestamps emitted by the logging API: if the regex doesn't match, it re-raises the original exception emitted by strptime. PTAL

@dhermes
Copy link
Contributor

dhermes commented Mar 10, 2016

Since when do we love using regex everywhere? My question about a guarantee about the number of digits after the decimal point is still unanswered. Do you know if it will always be 9? Do trailing 0's get chopped?

The goal of my feedback was to avoid having to catch the ValueError and that fix doesn't really capture it

@tseaver
Copy link
Contributor Author

tseaver commented Mar 10, 2016

Since when do we love using regex everywhere?

The regex gives us a way to parse the new format which is a) faster, b) easier to understand, c) less error prone than the manual parsing I initially added.

My question about a guarantee about the number of digits after the decimal point is still unanswered.

The docs promise "nanosecond precision", which requires 9 digits, even with trailing zeroes.

The goal of my feedback was to avoid having to catch the ValueError and that fix doesn't really capture it

We cannot do anything sensible with hypothetical malformed values from the backend, and the previous code didn't try to prevent them (in fact, this code comes from me exposing them when talking to the logging API). If we come across another timestamp format when mapping a new API, then we will have to adjust this helper again: I see that as a better alternative than trying to smother ValueErrors here.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 10, 2016

I have what might be a better idea: rather than overloading the existing _rfc3339_to_datetime to handle the new nanosecond format, I could just add a separate helper (_rfc3339_nanos_to_datetime), which would be used only for parsing timestamps specified as being nanosecond-precision.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 11, 2016

@dhermes PTAL again. I think splitting the two branches into separate functions worked out well.

I'll be glad to squash everything down before merging.

@dhermes
Copy link
Contributor

dhermes commented Mar 11, 2016

LGTM. Squashing commits would be nice if it isn't too much

@tseaver
Copy link
Contributor Author

tseaver commented Mar 11, 2016

Squashed. I will merge after Travis.

tseaver added a commit that referenced this pull request Mar 11, 2016
@tseaver tseaver merged commit 68baa1b into googleapis:logging-api Mar 11, 2016
@tseaver tseaver deleted the logging-client_list_entries branch March 11, 2016 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants