Skip to content

Improve BaseApiClient example #86

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

Closed
wants to merge 2 commits into from

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Sep 4, 2024

  • Improve the typing of the stub: the mypy-protobuf plugin generates a XxxAsyncStub class for each service, which have proper type information to be used with async. This still needs some hacks (type ignore) to make it work, but it's better than using the normal stubs and having to cast on each call.
  • Add channel_defaults.

The `mypy-protobuf` plugin generates a `XxxAsyncStub` class for each
service, which have proper type information to be used with async. This
still needs some hacks (type ignore) to make it work, but it's better
than using the normal stubs and having to cast on each call.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax requested a review from a team as a code owner September 4, 2024 16:30
@llucax llucax requested a review from Marenz September 4, 2024 16:30
@github-actions github-actions bot added the part:code Affects the code in general label Sep 4, 2024
@llucax llucax added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Sep 4, 2024
@llucax llucax enabled auto-merge September 4, 2024 16:31
@llucax llucax self-assigned this Sep 4, 2024
@llucax llucax added this to the v0.7.0 milestone Sep 4, 2024
@llucax llucax disabled auto-merge September 4, 2024 16:34
@llucax llucax changed the title Improve the typing of the example in BaseApiClient Improve BaseApiClient example Sep 4, 2024
@llucax llucax enabled auto-merge September 4, 2024 16:37
@llucax llucax added part:docs Affects the documentation and removed part:code Affects the code in general labels Sep 4, 2024
@llucax llucax marked this pull request as draft September 5, 2024 08:35
auto-merge was automatically disabled September 5, 2024 08:35

Pull request was converted to draft

@llucax
Copy link
Contributor Author

llucax commented Sep 5, 2024

Converting to draft because I'm finding issues at runtime with this approach, because the XxxAsyncStub doesn't really exist at runtime 😟

@llucax
Copy link
Contributor Author

llucax commented Sep 5, 2024

It looks like we need this accepted to make it work:

Otherwise we can't use the AsyncStub as a generic parameter when inheriting from BaseApiClient.

If this doesn't get updated and we want to still use the async stub, I guess we need to take out the stub from the BaseApiClient class and each sub-class will need to handle it manually on their own.

@llucax llucax added the status:blocked Other issues must be resolved before this can be worked on label Sep 5, 2024
@llucax
Copy link
Contributor Author

llucax commented Sep 5, 2024

A quick hack is:

    def __init__(...) -> None:
        super().__init__(
            server_url,
            microgrid_pb2_grpc.MicrogridStub,
            connect=connect,
            channel_defaults=channel_defaults,
        )
        self._async_stub: microgrid_pb2_grpc.MicrogridAsyncStub = self.stub  # type: ignore

And then just use self._async_stub internally.

@llucax
Copy link
Contributor Author

llucax commented Oct 29, 2024

Superseded by:

The improvement of the documentation to include defaults will be done in a separate PR.

@llucax llucax added the resolution:wontfix This will not be worked on label Oct 29, 2024
@llucax llucax closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd:skip-release-notes It is not necessary to update release notes for this PR part:docs Affects the documentation resolution:wontfix This will not be worked on status:blocked Other issues must be resolved before this can be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant