-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Restore 'Dataset' class. #660
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
Conversation
gcloud/datastore/dataset.py
Outdated
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 |
gcloud/datastore/dataset.py
Outdated
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.
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>
Includes: * tweaked logic around defining recursive message types * more sophisticated logic for generating unit tests using recursive message types * flattened map-y fields are handled properly * fixed a corner case where a method has a third-party request object and flattened fields
🤖 I have created a release \*beep\* \*boop\* --- ### [0.35.5](https://www.github.com/googleapis/gapic-generator-python/compare/v0.35.4...v0.35.5) (2020-10-19) ### Bug Fixes * numerous small fixes to allow bigtable-admin ([#660](https://www.github.com/googleapis/gapic-generator-python/issues/660)) ([09692c4](https://www.github.com/googleapis/gapic-generator-python/commit/09692c4e889ccde3b0ca31a5e8476c1679804beb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Wrapper around 'datastore.api.{get,put,delete}', plus the 'Key',
'Batch', 'Transaction', and 'Query' factories.
Per @jgeewax in #659.