feat: implement async rest mixin methods #2181
Conversation
ohmayr
force-pushed
the
implement-async-rest-mixins-call-method
branch
2 times, most recently
from
d2b45c9
to
831c55a
Compare
product-auto-label
bot
added
size: l
gapic/templates/%namespace/%name_%version/%sub/services/%service/_async_mixins.py.j2
Outdated
Show resolved
Hide resolved
ohmayr
force-pushed
the
refactor-mixins-callables
branch
2 times, most recently
from
abdba51
to
f0ccd61
Compare
ohmayr
force-pushed
the
implement-async-rest-mixins-call-method
branch
from
831c55a
to
931feda
Compare
product-auto-label
bot
added
size: xl
ohmayr
force-pushed
the
implement-async-rest-mixins-call-method
branch
from
931feda
to
4e52cd7
Compare
| @@ -265,6 +265,13 @@ def _prep_wrapped_messages(self, client_info): | |||
| client_info=client_info, | |||
| ), | |||
| {% endfor %}{# service.methods.values() #} | |||
| {% for method_name in api.mixin_api_methods.keys() %} | |||
There was a problem hiding this comment.
We don't need transport_safe_name? Seems like it would be a good practice to use, like in the loop above. . Are the values of the api_methods dict Method objects, so we could do that? Otherwise, no worries. We deal with mixin methods individually, so we can know when a problem arises.
There was a problem hiding this comment.
I'm not sure if transport_safe_name is available in api.mixin_api_methods.keys() . I've filed and linked an issue to follow up on this.
gapic/templates/%namespace/%name_%version/%sub/services/%service/_async_mixins.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/_async_mixins.py.j2
Outdated
Show resolved
Hide resolved
| default_timeout=None, | ||
| client_info=DEFAULT_CLIENT_INFO, | ||
| ) | ||
| rpc = self.transport._wrapped_methods[self._client._transport.get_iam_policy] |
There was a problem hiding this comment.
Why do we have a call to wrapped_methods[get_iam_policy] both here in in _async_mixins.py.j2? I realize this was here before, but it's a genuine question. Does IAM have special treatment for historical reasons from before we used mixins?
There was a problem hiding this comment.
I updated all the instances just because it makes sense to use wraped_methods everywhere. However, the methods defined in async client aren't generated / used. There's a linked PR which mentions that those methods should be cleaned up. We can follow up on it. Thanks for filing the issue!
| @@ -388,7 +388,7 @@ class {{ service.grpc_asyncio_transport_name }}({{ service.name }}Transport): | |||
| return self._stubs["test_iam_permissions"] | |||
| {% endif %} | |||
|
|
|||
| {{ shared_macros.prep_wrapped_messages_async_method(service)|indent(4) }} | |||
There was a problem hiding this comment.
I'm concerned in that higher up in this functions, we add set_iam_policy if opts.add_iam_methods. But the TODO above {% if opts.add_iam_methods %} makes it seem like that may not be used any more. WDYT?
Incidentally, I just filed #2196 to remove these TODOs.
There was a problem hiding this comment.
You're right, the methods defined within async client aren't used. The ones defined in async mixins file are.
...ation/goldens/credentials/google/iam/credentials_v1/services/iam_credentials/async_client.py
Outdated
Show resolved
Hide resolved
...egration/goldens/redis/google/cloud/redis_v1/services/cloud_redis/transports/rest_asyncio.py
Outdated
Show resolved
Hide resolved
| @@ -225,6 +225,9 @@ class Async{{service.name}}RestTransport(_Base{{ service.name }}RestTransport): | |||
| return self._{{method.name}}(self._session, self._host, self._interceptor) # type: ignore | |||
|
|
|||
| {% endfor %} | |||
| {% for name, sig in api.mixin_api_signatures.items() %} | |||
| {{ shared_macros.generate_mixin_call_method(service, api, name, sig, is_async=True) | indent(4) }} | |||
There was a problem hiding this comment.
This doesn't need to block, but would be a good follow-up: this macro is defining the each _Mixin class next to the corresponding @property. The other _Method classes are defined all together, and then their @property all together. I think it would be good to define the _mixin classes right after the _Method classes, and the mixin @property right after the method @property. WDYT? Could you file an issue so we don't forget?
There was a problem hiding this comment.
I've filed and linked an issue to follow up on this.
Fixes: #2148, #2156