-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Flesh out bigquery API #1045
Flesh out bigquery API #1045
Conversation
:rtype: :class:`gcloud.bigquery.dataset.Dataset` | ||
:returns: Dataset parsed from ``resource``. | ||
""" | ||
name = resource['datasetReference']['datasetId'] |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Really easy to review! Thanks. Only a few hangups, |
Addresses: https://github.com/GoogleCloudPlatform/gcloud-python/pull/1045/files#r36571762 https://github.com/GoogleCloudPlatform/gcloud-python/pull/1045/files#r36571777 https://github.com/GoogleCloudPlatform/gcloud-python/pull/1045/files#r36571768 https://github.com/GoogleCloudPlatform/gcloud-python/pull/1045/files#r36571771
@dhermes I think everything is resolved, except the question about
|
"""List datasets for the project associated with this client. | ||
|
||
See: | ||
https://cloud.google.com/bigquery/reference/rest/v1beta2/projects/datasets/list |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
I agree that we can be mostly secure in the belief that the backend will send good data and that without those keys, we can't do anything anyhow. I just wanted to discuss the possibility that we would provide a more specific error message than |
Addresses: #1045 (comment) #1045 (comment)
Addresses: #1045 (comment)
FWIW, I'd generally prefer not to re-wrap exceptions (losing the original traceback really damages debuggability, for instance). |
If we raise one line after the |
Hiding the key error is the harm I'm talking about: the user won't be better able to debug some other error more easily than "that key isn't in the resource as expected". |
Maybe we are thinking of different things. This is what I have in mind: if ('datasetReference' not in resource or
'datasetId' not in resource['datasetReference']):
raise KeyError('The resource returned from the server did not contain '
'the the necessary information to create a Dataset '
'object. The resource needs to contain a dictionary value '
'at the datasetReference key and within that dictionary '
'needs the datasetId.')
name = resource['datasetReference']['datasetId'] What harm does this cause? I'm just suggesting we provide more information than what a |
When a Python programmer sees a traceback for a name = resource['datasetReference']['datasetId'] doesn't she already know the same information you typed into that waaay-long error message? If debugging it, she will still dump out the contents of |
Yes that's the root of my original question. Do we want that Python programmer to just see that |
LGTM |
Client.list_datasets
andDataset.list_tables
methods.