Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Jan 14, 2015

For hygeine:

  • Hide batch._BATCHES.top behind batch.Batch.current() classmethod.

Per #514:

  • Remove `Connection.save_entity()``
  • Remove Connection.delete_entities()
  • Remove Connection.mutation
  • Remove Connection.transaction
    • Make Connection.lookup and Connection.run_query depend on transaction
      on top of the _BATCHES stack.

@dhermes dhermes mentioned this pull request Jan 14, 2015
8 tasks
@dhermes
Copy link
Contributor

dhermes commented Jan 14, 2015

Does this depend on #548? I have yet to view the code, but is

Make Connection.lookup and Connection.run_query depend on transaction on top
of the _BATCHES stack.

meant to be a temporary replacement for the interaction of Transaction and Connection or is is meant to be permanent?

I was imagining a world in which Connection was "stateless" and relied on the callers to tell it how to send the RPCs. Really just a stand-alone mapping for DatastoreService'.

@tseaver
Copy link
Contributor Author

tseaver commented Jan 14, 2015

Yes, based on #548.

To accomplish what you are asking, we would need to have Connection.lookup and Connection.run_query take a transaction_id argument (like Connection.commit).

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b1935fe on tseaver:514-rip_out_connection_cruft into f980aad on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor

dhermes commented Jan 14, 2015

That doesn't seem like too big an issue. Are you opposed to that?

@tseaver
Copy link
Contributor Author

tseaver commented Jan 14, 2015

It means moving the logic we currently have in Connection._set_read_options somewhere else (maybe a helper called by a new top-level lookup() API, as well as from the query iterator?)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 76a918e on tseaver:514-rip_out_connection_cruft into f980aad on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented Jan 14, 2015

Rebased after merging #548.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c449af6 on tseaver:514-rip_out_connection_cruft into * on GoogleCloudPlatform:master*.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c449af6 on tseaver:514-rip_out_connection_cruft into 64aaed4 on GoogleCloudPlatform:master.

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 Jan 14, 2015

OK I did a commit by commit review and even managed to put my comments on the PR instead of the commits (pats self on back).

This mostly looks good except I think you should refactor Connection.lookup, Connection.run_query and connection._set_read_options to take a transaction ID and then make sure that api.get passes that ID to Connection.lookup and query.Iterator.next_page passes that ID to Connection.run_query.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0aa8804 on tseaver:514-rip_out_connection_cruft into 64aaed4 on GoogleCloudPlatform:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f10620e on tseaver:514-rip_out_connection_cruft into 64aaed4 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented Jan 15, 2015

@dhermes the following two commits do as you recommend:

  • 301498d adds a Transaction.current() class method, which returns the top-most item on the batch
    stack IFF it is a tranaction (otherwise, None).
  • a4a65a2 makes the Connection methods take an explicit transaction_id, and uses the Transaction.current() bit to find it in the callers (datasore.get() and QueryIterator.next_page())

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 15, 2015
@dhermes
Copy link
Contributor

dhermes commented Jan 15, 2015

LGTM w00t! (Sorry I am reviewing from phone.)

tseaver added a commit that referenced this pull request Jan 15, 2015
@tseaver tseaver merged commit 12ac983 into googleapis:master Jan 15, 2015
@tseaver tseaver deleted the 514-rip_out_connection_cruft branch January 15, 2015 03:14
parthea pushed a commit that referenced this pull request Sep 22, 2023
Source-Link: https://togithub.com/googleapis/synthtool/commit/0ddbff8012e47cde4462fe3f9feab01fbc4cdfd6
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:bced5ca77c4dda0fd2f5d845d4035fc3c5d3d6b81f245246a36aee114970082b
parthea pushed a commit that referenced this pull request Oct 21, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Oct 21, 2023
Source-Link: googleapis/synthtool@352b9d4
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:3e3800bb100af5d7f9e810d48212b37812c1856d20ffeafb99ebe66461b61fc7

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea added a commit that referenced this pull request Sep 18, 2025
* chore(deps): update all dependencies

* fix(deps): allow pyarrow < 10

* 🦉 Updates from OwlBot post-processor

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

* 🦉 Updates from OwlBot post-processor

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

* chore: update dependency google-cloud-bigquery==3.3.1

Co-authored-by: Anthonios Partheniou <partheniou@google.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Nov 22, 2025
Source-Link: googleapis/synthtool@06e8279
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:b3500c053313dc34e07b1632ba9e4e589f4f77036a7cf39e1fe8906811ae0fce
parthea pushed a commit that referenced this pull request Nov 24, 2025
Otherwise this file is excluded from generation. Partially addresses #437
parthea pushed a commit that referenced this pull request Nov 24, 2025
parthea added a commit that referenced this pull request Nov 24, 2025
* chore: update templated files

* remove obsolete replacements in owlbot.py

* update replacements in owlbot.py

* 🦉 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>
Co-authored-by: Kevin Zheng <147537668+gkevinzheng@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Nov 24, 2025
* build(python): use release-publish app for notifying GitHub of release status

* fix: re-add pypi password

Source-Author: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>
Source-Date: Wed Sep 16 08:46:42 2020 -0600
Source-Repo: googleapis/synthtool
Source-Sha: 257fda18168bedb76985024bd198ed1725485488
Source-Link: googleapis/synthtool@257fda1

* build(python): add secret manager in kokoro

Source-Author: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>
Source-Date: Wed Sep 16 10:24:40 2020 -0600
Source-Repo: googleapis/synthtool
Source-Sha: dba48bb9bc6959c232bec9150ac6313b608fe7bd
Source-Link: googleapis/synthtool@dba48bb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.