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

feat: add support for context manager in client #987

Merged

atulep merged 15 commits into googleapis:master from atulep:master

Oct 4, 2021

Contributor

atulep commented Sep 7, 2021

Addresses feature request filed in #575.

atulep added 2 commits

September 7, 2021 10:58


          feat: add support for context manager in client.

fd9148f


          chore: remove extra whitespace.

b677108

atulep requested a review from a team as a code owner

September 7, 2021 18:04

google-cla bot added the cla: yes label

software-dov self-requested a review

September 7, 2021 18:10

atulep linked an issue

that may be closed by this pull request

Feature Request: allow clients to act as context manager and add close() method to cleanup resources #575

Closed

software-dov reviewed

View reviewed changes

Contributor

software-dov left a comment

Looks good, just some nits around testing.

I think it would be good to have some autogenerated unit tests as well so we can keep generated code coverage at 100%.
Maybe something that injects a dummy transport that can tell us that it was told to enter, something like (strawman expressions in the template, use the correct ones :P )

class DummyTransport:
    def __init__(self):
        self.trigger_state = 0
        
    def __enter__(self):
        self.trigger_state = 1
        
    def __exit__(self, *args, **kwargs):
        self.trigger_state = 2
        
client = {{ api.module_name }}.{{ service.client_name }}(transport=DummyTransport())
assert c.transport.trigger_state == 0
with client:
    assert c.transport.trigger_state == 1
    
assert c.transport.trigger_state == 2

And then associated mox tests for the grpc and rest transport enter/exit methods (I always have to steal mox setup from existing code).

gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 Show resolved Hide resolved

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 Outdated Show resolved Hide resolved

atulep added 4 commits

September 8, 2021 15:44


          chore: adds autogenerated unit tests.

15b2533


          chore: adds stronger warning.

f5dc286


          chore: fixes tests.

c560997


          chore: adds auto-generated tests for ads.

983888b

Contributor Author

atulep commented Sep 13, 2021

Looks like golden files need to be updated. I can't get bazel run :update on my Mac due to some weird error related to wrong version of Python. I'm investigating it now.


          chore: updates golden files.

3e49e1a

atulep requested a review from busunkim96

September 13, 2021 21:05

software-dov reviewed

View reviewed changes

Contributor

software-dov left a comment

Looks good, just a few more obnoxious nitpicking comments :)

gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Show resolved Hide resolved

gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved

gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved


          chore: refactor.

b00a6a6

software-dov reviewed

View reviewed changes

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved


          chore: refactor.

77d92c7

busunkim96 requested a review from tswast

September 16, 2021 15:08

busunkim96 approved these changes

View reviewed changes

Contributor

busunkim96 left a comment

Thank you for implementing this!

CC @tswast @nicain I think both of you were interested in this feature, PTAL if you have a moment.

tseaver reviewed

View reviewed changes

gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 Show resolved Hide resolved

tswast reviewed

View reviewed changes

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 Outdated Show resolved Hide resolved

gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 Outdated Show resolved Hide resolved

ncclementi mentioned this pull request

Initial read_gbq support coiled/dask-bigquery#4

Merged


          feat: adds close() to transport and ctx to async client.

4707af5

Contributor Author

atulep commented Sep 23, 2021

@lidizheng PTAL async client

lidizheng reviewed

View reviewed changes

Contributor

lidizheng left a comment

The added context manager method looks good, just one minor comment.

gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 Show resolved Hide resolved


          feat: adds close method and removes ctx from transport in ads.

a136e61

Contributor Author

atulep commented Sep 24, 2021

@software-dov @tseaver @tswast @busunkim96 PTAL. I've addressed all your comments.

atulep added 2 commits

September 27, 2021 11:25


          chore: adds warning infobox to docstring.

3ab1200


          chore: updates integration tests.

db48daf

tswast reviewed

View reviewed changes

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/base.py.j2 Outdated Show resolved Hide resolved

gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/transports/base.py.j2 Outdated

+                  def close(self):
+                      """Closes resources associated with the transport.
+                      :: warning::

tswast Sep 27, 2021

Suggested change

      
                    :: warning::
          
                    .. warning::

tests/integration/goldens/asset/google/cloud/asset_v1/services/asset_service/transports/base.py Outdated

+                  def close(self):
+                      """Closes resources associated with the transport.
+                     :: warning::

tswast Sep 27, 2021

Suggested change

      
                   :: warning::
          
                   .. warning::

...on/goldens/credentials/google/iam/credentials_v1/services/iam_credentials/transports/base.py Outdated

+                  def close(self):
+                      """Closes resources associated with the transport.
+                     :: warning::

tswast Sep 27, 2021

Suggested change

      
                   :: warning::
          
                   .. warning::

...ation/goldens/logging/google/cloud/logging_v2/services/logging_service_v2/transports/base.py Outdated

+                  def close(self):
+                      """Closes resources associated with the transport.
+                     :: warning::

tswast Sep 27, 2021

Suggested change

      
                   :: warning::
          
                   .. warning::

...ation/goldens/logging/google/cloud/logging_v2/services/metrics_service_v2/transports/base.py Outdated

+                  def close(self):
+                      """Closes resources associated with the transport.
+                     :: warning::

tswast Sep 27, 2021

Suggested change

      
                   :: warning::
          
                   .. warning::

tests/integration/goldens/redis/google/cloud/redis_v1/services/cloud_redis/transports/base.py Outdated

+                  def close(self):
+                      """Closes resources associated with the transport.
+                     :: warning::

tswast Sep 27, 2021

Suggested change

      
                   :: warning::
          
                   .. warning::

...ration/goldens/logging/google/cloud/logging_v2/services/config_service_v2/transports/base.py Outdated

+                  def close(self):
+                      """Closes resources associated with the transport.
+                     :: warning::

tswast Sep 27, 2021

Suggested change

      
                   :: warning::
          
                   .. warning::

software-dov approved these changes

View reviewed changes

atulep and others added 2 commits

September 30, 2021 10:38


          chore: fixes typo.

73f162c


          Merge branch 'master' into master

d169bc2

atulep merged commit 4edabcf into googleapis:master

This was referenced Jan 18, 2022

chore(master): release reverted #1133

Merged

chore(master): release 0.40.0 #1138

Closed

chore(master): release 0.39.1 #1146

Closed

This was referenced Jan 26, 2022

chore(main): release 0.39.1 #1160

Closed

chore(main): release 0.39.1 #1162

Closed

chore(main): release 0.61.0 #1163

Merged

parthea mentioned this pull request

[BigQuery Data Transfer Service, all GAPIC clients]: provide a close() method and context manager to clean up client-owned resources googleapis/google-cloud-python#9457

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

busunkim96 busunkim96 approved these changes

lidizheng lidizheng left review comments

tswast tswast left review comments

tseaver tseaver left review comments

software-dov software-dov approved these changes

Labels