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.

Sign up for GitHub

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

Jump to bottom

Adding lazy loading support for datastore connection. #668

Merged
merged 4 commits into from
Feb 25, 2015
Merged

Adding lazy loading support for datastore connection. #668

merged 4 commits into from
Feb 25, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 19, 2015

NOTE: Has #667 as diffbase.

Happens in 4 phases:

  • Remove datastore.helpers import in connection. (this would have caused a cycle if Connection was used in _implicit_environ)
  • Moving get_connection() from datastore.__init__ to _implicit_environ.
  • Moving set_default_connection() from datastore.__init__ to _implicit_environ.
  • Adding lazy loading support for the connection property

Note that I had to copy _prepare_key_for_request from helpers into connection to avoid cyclic imports (#528 would be nice, since we could just remove _prepare_key_for_request all together)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 19, 2015
@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Feb 19, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fa04127 on dhermes:lazy-loading-attempt-5 into 72b6359 on GoogleCloudPlatform:master.

@dhermes dhermes mentioned this pull request Feb 19, 2015
Closed
tseaver
tseaver reviewed Feb 19, 2015
View reviewed changes
gcloud/datastore/_implicit_environ.py
:param name: The name of the attribute / property being evaluated.

:type method: callable that takes no arguments
:param method: The method used to evaluate the property.

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Feb 19, 2015

The last commit LGTM, except for my note about the _method name.

@dhermes dhermes mentioned this pull request Feb 19, 2015
Merged
@dhermes
Remove datastore.helpers import in connection.
77f19bd 
Done via:
- Moving `_add_keys_to_request` definition to connection module
- Removing only other use (in `batch`, was not a list of keys)
- Copying `_prepare_key_for_request` from helpers (googleapis#528 would be
  nice, since we could just remove `_prepare_key_for_request`
  all together)
@dhermes
Moving get_connection() from datastore.__init__ to _implicit_environ.
7be6a1d
@dhermes
Moving set_default_connection() to _implicit_environ.
d0fb459 
Moved from datastore.__init__.
@dhermes
Adding lazy loading support for datastore connection.
7b0fc6e
@dhermes
Copy link
Contributor Author

dhermes commented Feb 19, 2015

@tseaver PTAL I rebased on top of #667.

Your _method comment was resolved in #667. You gave the LGTM for the final commit, but this PR actually involved 3 commits to move code before (on of which involves a duplicated method and pragma NO COVER worth discussing)

Also the commit you LGTM'd changed slightly since I changed #667 to use a decorator for lazy properties

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7b0fc6e on dhermes:lazy-loading-attempt-5 into 56f60bb on GoogleCloudPlatform:master.

tseaver
tseaver reviewed Feb 19, 2015
View reviewed changes
gcloud/datastore/batch.py
@@ -189,8 +189,8 @@ def delete(self, key):
if not _dataset_ids_equal(self._dataset_id, key.dataset_id):
raise ValueError("Key must be from same dataset as batch")

key_pb = key.to_protobuf()
helpers._add_keys_to_request(self.mutation.delete, [key_pb])
key_pb = helpers._prepare_key_for_request(key.to_protobuf())

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Feb 19, 2015

I've got to run, won't be able to finish review tonight. :(

@dhermes
Copy link
Contributor Author

dhermes commented Feb 19, 2015

No worries

This was referenced Feb 20, 2015
Merged
Merged
@dhermes
Copy link
Contributor Author

dhermes commented Feb 24, 2015

@tseaver After this is in I'd like to cut 0.4.2. WDYT?

@tseaver
Copy link
Contributor

tseaver commented Feb 25, 2015

OK, LGTM. 0.4.2 would be fine, as well.

dhermes added a commit that referenced this pull request Feb 25, 2015
@dhermes
Merge pull request #668 from dhermes/lazy-loading-attempt-5
0e41dc3 
Adding lazy loading support for datastore connection.
@dhermes dhermes merged commit 0e41dc3 into googleapis:master Feb 25, 2015
@dhermes dhermes deleted the lazy-loading-attempt-5 branch February 25, 2015 02:13
@dhermes dhermes mentioned this pull request Feb 25, 2015
Merged
parthea pushed a commit that referenced this pull request Jul 6, 2023
@jmdobry @busunkim96
Add Cloud Client NL API samples. [(#668)](GoogleCloudPlatform/python-…
464ac55 
…docs-samples#668)
vchudnov-g pushed a commit that referenced this pull request Sep 20, 2023
@gcf-owl-bot
feat: added speech endpointing setting (#668)
2da7a77 
* feat: added speech endpointing setting
feat: added Knowledge Search API

PiperOrigin-RevId: 560215389

Source-Link: googleapis/googleapis@b1666e6

Source-Link: googleapis/googleapis-gen@07e82b4
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMDdlODJiNGQ0OTE3NDk0ZDg1MDM5NzIyZjY3M2Q0MDljNGM3MzA1ZiJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api: datastore Issues related to the Datastore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dhermes @coveralls @tseaver @googlebot