-
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
Restore 'Dataset' class. #660
Conversation
|
||
|
||
class Dataset(object): | ||
|
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.
LGTM pending Travis / comment We should let @jgeewax sign off as well. I don't like the factories but it makes sense in context and is certainly the lowest overhead answer |
Slight retraction of LGTM: we should have an optional connection on the object (Reviewing from phone) |
Changes Unknown when pulling c368db3 on tseaver:659-dataset-revenant into * on GoogleCloudPlatform:master*. |
@dhermes if the user passes no connection to the ctor, would you see it looking one up from the implicit environ, or just passing None through to the wrapped APIs? |
Pass it through to the API methods, rather than having the caller pass it. Don't try to resolve it if None is passed (the API methods do that for us).
317ddfd assumes the second option I described. |
Changes Unknown when pulling 317ddfd on tseaver:659-dataset-revenant into * on GoogleCloudPlatform:master*. |
@tseaver just wrapping up teaching for the day. I did mean the option you chose but did not mean that connection should be removed from the signatures. However after thinking about it, ISTM that is the right choice. I.E. the dataset should not have a connection overridden as an option, so either pass to constructor or never use. |
After this is in I'll try to put together my vision for lazy loaded defaults |
[ci skip]
@jgeewax PTAL |
Passes our ``dataset_id``. | ||
""" | ||
dataset_id = kwargs.pop('dataset_id', None) | ||
if dataset_id not in (None, self.dataset_id): |
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.
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.
Passes our ``dataset_id``. | ||
""" | ||
if 'dataset_id' in kwargs: | ||
raise TypeError('Cannot pass dataset_id') |
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.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Changes Unknown when pulling 3d2699e on tseaver:659-dataset-revenant into * on GoogleCloudPlatform:master*. |
LGTM pending Sphinx links to the classes. We can merge after, yes? |
[ci skip]
@tseaver Can you give an FYI / heads up when using |
@tseaver I'm gonna to submit a bunch of bite-sized PRs removing |
Re: |
Re: the PRs moving |
Just an "FYI: docstring only change, I used [ci skip]" (I agree it's totally valid) |
This looks great guys. |
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Wrapper around 'datastore.api.{get,put,delete}', plus the 'Key',
'Batch', 'Transaction', and 'Query' factories.
Per @jgeewax in #659.