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

Remove generic type from BaseApiClient #92

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Oct 29, 2024

The BaseApiClient class is not generic anymore, and doesn't take a function to create the stub. Instead, subclasses should create their own stub right after calling the parent constructor. This enables subclasses to cast the stub to the generated XxxAsyncStub class, which have proper async type hints.

@llucax llucax requested a review from a team as a code owner October 29, 2024 10:43
@github-actions github-actions bot added part:docs Affects the documentation part:code Affects the code in general labels Oct 29, 2024
@llucax llucax requested a review from shsms October 29, 2024 10:55
@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Oct 29, 2024
Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

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

Just a few minor comments to check for

RELEASE_NOTES.md Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
but in the `.pyi` file, so the type can be used to specify type hints, but
**not** in any other context, as the class doesn't really exist for the Python
interpreter. This include generics, and because of this, this class can't be
even parametrized using the async class, so the instantiation of the stub can't
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
even parametrized using the async class, so the instantiation of the stub can't
even parameterized using the async class, so the instantiation of the stub can't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I found it confusing :D
I seems I was wrong 👼

Copy link
Contributor

Choose a reason for hiding this comment

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

it is an irregular language.

Copy link
Contributor Author

@llucax llucax Oct 29, 2024

Choose a reason for hiding this comment

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

I just noticed that it says parametrized when defining past tense and past participle, but uses parameterized in the example 🤣

image

@llucax llucax self-assigned this Oct 29, 2024
@llucax llucax added this to the v0.7.0 milestone Oct 29, 2024
The `BaseApiClient` class is not generic anymore, and doesn't take a
function to create the stub. Instead, subclasses should create their own
stub right after calling the parent constructor. This enables subclasses
to cast the stub to the generated `XxxAsyncStub` class, which have
proper `async` type hints.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Copy link
Contributor Author

llucax commented Oct 29, 2024

Rebased to resolve release notes conflicts too.

@llucax llucax added scope:breaking-change Breaking change, users will need to update their code type:enhancement New feature or enhancement visitble to users labels Oct 29, 2024
@llucax llucax enabled auto-merge October 29, 2024 11:04
@llucax
Copy link
Contributor Author

llucax commented Oct 29, 2024

Enabled auto-merge.

@llucax llucax disabled auto-merge October 29, 2024 11:08
Comment on lines +32 to +36
class MyApiClient(BaseApiClient):
def __init__(self, server_url: str, *, ...) -> None:
super().__init__(server_url, connect=connect)
self._stub = cast(MyServiceAsyncStub, MyServiceStub(self.channel))
...
Copy link
Contributor

@shsms shsms Oct 29, 2024

Choose a reason for hiding this comment

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

Can't we just ask them to pass cast(MyServiceAsyncStub, MyServiceStub(service.channel)) as an argument, and make the base client generic over MyServiceAsyncStub instead?

Copy link
Contributor Author

@llucax llucax Oct 29, 2024

Choose a reason for hiding this comment

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

No, because __init__ can't have MyServiceAsyncStub as a type hint, to do that we need to take the type as a generic, and we can't use an async stub as a generic because it is parsed by the interpreter, and it can't find it because it is not present in the .py file, only the .pyi file.

This is how great grpio is handling asyncio and type-hinting 😬 (there is a new experimental interface in the cooking, let's see how that turns out).

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't parse them with from __future__ import annotations right?

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind, it will parse them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because it is not an annotation in this case, is is a proper object for the interpreter. You can give it a go if you want, I already tried many different attempts and nothing seemed to work.

$ head -n 100 t.py* t2.py 
==> t.py <==
class IDoExist:
    pass

==> t.pyi <==
class IDoExist:
    pass

class IDontExist:
    pass

==> t2.py <==
from __future__ import annotations

from typing import Generic, TypeVar

from t import IDoExist, IDontExist

T = TypeVar("T")


class G(Generic[T]):
    def __init__(self, x: T):
        self.x = x


class Sub(G[IDoExist]):
    pass


class Sub2(G[IDontExist]):
    pass
$ python t2.py 
Traceback (most recent call last):
  File "/home/luca/devel/client-base/t2.py", line 5, in <module>
    from t import IDoExist, IDontExist
ImportError: cannot import name 'IDontExist' from 't' (/home/luca/devel/client-base/t.py)

Copy link
Contributor Author

@llucax llucax Oct 29, 2024

Choose a reason for hiding this comment

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

Put another way, with TYPE_CHECKING:

from t import IDoExist

if TYPE_CHECKING:
    from t import IDontExist

Then the error becomes:

$ python t2.py 
Traceback (most recent call last):
  File "/home/luca/devel/client-base/t2.py", line 22, in <module>
    class Sub2(G[IDontExist]):
                 ^^^^^^^^^^
NameError: name 'IDontExist' is not defined. Did you mean: 'IDoExist'?

Maybe it is more clear to see this way. Only stuff after a : (or used in special constructs, like cast), are type hints, the rest is just code.

@llucax
Copy link
Contributor Author

llucax commented Oct 29, 2024

I just added back the commit in #86 adding the channels defaults to the examples too.

@llucax llucax enabled auto-merge October 29, 2024 11:10
@llucax llucax added this pull request to the merge queue Oct 29, 2024
@daniel-zullo-frequenz daniel-zullo-frequenz removed this pull request from the merge queue due to a manual request Oct 29, 2024
@daniel-zullo-frequenz
Copy link
Contributor

I've removed it from the queue because Sahas had some comments

@llucax llucax added this pull request to the merge queue Oct 29, 2024
@llucax llucax removed this pull request from the merge queue due to a manual request Oct 29, 2024
@llucax llucax dismissed daniel-zullo-frequenz’s stale review October 29, 2024 11:14

Dismissing only to be able to enable auto-merge again, and let Sahas make the final approval that triggers the merging

@llucax llucax added this pull request to the merge queue Oct 29, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit 5414a6d Oct 29, 2024
14 checks passed
@llucax llucax deleted the no-stub branch October 29, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:code Affects the code in general part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests scope:breaking-change Breaking change, users will need to update their code type:enhancement New feature or enhancement visitble to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants