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.

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

Fix #260: break import cycles in datastore w/o factories. #288

Closed
wants to merge 1 commit into from
Closed

Fix #260: break import cycles in datastore w/o factories. #288

wants to merge 1 commit into from

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Oct 23, 2014

Finishes fix for #260 for datastore (and enforces cycle check in pylint).

Alternative fix to the 'factories' indirection in #268.

Note that this requires adding a messy arg to the 'Entity.from_protobuf'
classmethod, in order to avoid the need to import '_helpers'.

Another way to avoid this cycle would be to make 'Entity.from_protobuf' into a
free function in another module (likely also moving 'Key.from_protobuf' for
the sake of consistency).

Alternative fix to the 'factories' indirection in #268.

Note that this requires adding a messy arg to the 'Entity.from_protobuf'
classmethod, in order to avoid the need to import '_helpers'.

Another way to avoid this cycle would be to make 'Entity.from_protobuf' into a
free function in another module (likely also moving 'Key.from_protobuf' for
the sake of consistency).
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling edac223 on tseaver:260-break_datastore_cycles_wo_factories into 1e60e64 on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor

dhermes commented Oct 23, 2014

This makes me much more comfortable than #268 :)

I think one could argue both sides for putting the from_protobuf methods in another module, but I think the benefits outweigh the costs.

The only cost is the loss of convenience of not having them as classmethods on the object.

The benefit is no cyclic dependencies and simpler to maintain code, i.e. no need to pass around a _get_value function which should be unique anyhow.

I think the loss of convenience as classmethods is not so big a deal because

  • End users won't likely care about a from_protobuf method. Many end users may not even know that proto is the underlying serialization format.
  • Those that do know will be able to find these methods in another module.

@tseaver
Copy link
Contributor Author

tseaver commented Oct 23, 2014

I thought about moving the 'from_protobuf' implementations to '_helpers' (where all the related marshalling code lives), but that module is explicitly non-API. We might have to rename it and make at least those two functions APIs.

@dhermes
Copy link
Contributor

dhermes commented Oct 23, 2014

SGTM with a rename _helpers -> helpers. I suppose that was bound to happen anyhow.

@tseaver tseaver added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 23, 2014
@tseaver tseaver closed this Oct 23, 2014
@tseaver tseaver deleted the 260-break_datastore_cycles_wo_factories branch October 23, 2014 19:00
parthea pushed a commit that referenced this pull request Jun 4, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Jun 4, 2023
Source-Link: googleapis/synthtool@c4dd595
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:ce3c1686bc81145c81dd269bd12c4025c6b275b22d14641358827334fddb1d72
parthea added a commit that referenced this pull request Jun 4, 2023
* chore(deps): update all dependencies

* revert

Co-authored-by: Anthonios Partheniou <partheniou@google.com>
parthea pushed a commit that referenced this pull request Jul 6, 2023
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 Aug 15, 2023
Source-Link: googleapis/synthtool@eb78c98
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:8a5d3f6a2e43ed8293f34e06a2f56931d1e88a2694c3bb11b15df4eb256ad163

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Aug 15, 2023
)

Source-Link: https://github.com/googleapis/synthtool/commit/d6103f4a3540ba60f633a9e25c37ec5fe7e6286d
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:39f0f3f2be02ef036e297e376fe3b6256775576da8a6ccb1d5eeb80f4c8bf8fb
parthea added a commit that referenced this pull request Sep 20, 2023
…rrors. (#288)

Co-authored-by: Anthonios Partheniou <partheniou@google.com>
parthea pushed a commit that referenced this pull request Sep 20, 2023
…reams is now supported (#288)

* feat: Specifying language code and display name for text and audio streams is now supported

PiperOrigin-RevId: 513138925

Source-Link: googleapis/googleapis@187d780

Source-Link: googleapis/googleapis-gen@b7979ed
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYjc5NzllZDE4NjU0NTljNWQxOTRkYzU4MjZhOWNkZjFjZjg3NzU4MSJ9

* 🦉 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>
parthea pushed a commit that referenced this pull request Sep 22, 2023
* chore: update copyright year to 2022

PiperOrigin-RevId: 431037888

Source-Link: googleapis/googleapis@b3397f5

Source-Link: googleapis/googleapis-gen@510b54e
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNTEwYjU0ZTFjZGVmZDUzMTczOTg0ZGYxNjY0NTA4MTMwOGZlODk3ZSJ9

* 🦉 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

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Sep 22, 2023
Source-Link: googleapis/synthtool@1b9ad76
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:9db98b055a7f8bd82351238ccaacfd3cda58cdf73012ab58b8da146368330021
parthea pushed a commit that referenced this pull request Sep 22, 2023
Source-Link: https://github.com/googleapis/synthtool/commit/eaef28efd179e6eeb9f4e9bf697530d074a6f3b9
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:f8ca7655fa8a449cadcabcbce4054f593dcbae7aeeab34aa3fcc8b5cf7a93c9e
parthea added a commit that referenced this pull request Sep 22, 2023
* chore: exclude requirements.txt file from renovate-bot

Source-Link: googleapis/synthtool@f58d313
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:7a40313731a7cb1454eef6b33d3446ebb121836738dc3ab3d2d3ded5268c35b6

* update constraints files

* fix(deps): require protobuf 3.20.2

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
parthea added a commit that referenced this pull request Sep 22, 2023
fix(deps): require proto-plus >= 1.22.0
parthea added a commit that referenced this pull request Oct 21, 2023
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
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@52aef91
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:36a95b8f494e4674dc9eee9af98961293b51b86b3649942aac800ae6c1f796d4

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
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 added a commit that referenced this pull request Oct 21, 2023
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
parthea pushed a commit that referenced this pull request Oct 22, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants