Skip to content

Conversation

atulep
Copy link
Contributor

@atulep atulep commented Sep 7, 2021

Addresses feature request filed in #575.

@atulep atulep requested a review from a team as a code owner September 7, 2021 18:04
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 7, 2021
@software-dov software-dov self-requested a review September 7, 2021 18:10
Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@atulep
Copy link
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.

@atulep atulep requested a review from busunkim96 September 13, 2021 21:05
Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@busunkim96 busunkim96 requested a review from tswast September 16, 2021 15:08
Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for implementing this!

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

@atulep
Copy link
Contributor Author

atulep commented Sep 23, 2021

@lidizheng PTAL async client

Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@atulep
Copy link
Contributor Author

atulep commented Sep 24, 2021

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

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

:: warning::
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:: warning::
.. warning::

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:: warning::
.. warning::

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:: warning::
.. warning::

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:: warning::
.. warning::

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:: warning::
.. warning::

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:: warning::
.. warning::

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:: warning::
.. warning::

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.

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

6 participants