refactor: add proper return types to resource sequence methods#368
refactor: add proper return types to resource sequence methods#368
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
tdstein
force-pushed
the
tdstein/fix-protocol-return-types
branch
from
9a22be7 to
39e1eb0
Compare
| path = posixpath.join(self._path, uid) | ||
| return _Resource(self._ctx, path, **result) | ||
| resource = self._factory(self._ctx, path, **result) | ||
| resource = cast(_T, resource) |
There was a problem hiding this comment.
I want to avoid this cast, but I haven't figured it out yet...
There was a problem hiding this comment.
I couldn't either... I tried to do this locally before you opened the PR. 😜
schloerke
left a comment
schloerke
left a comment
There was a problem hiding this comment.
Couple typing style requests.
One question on the covariant type var.
| self, | ||
| ctx: Context, | ||
| path: str, | ||
| factory: ResourceFactory[_T_co] = _Resource, |
There was a problem hiding this comment.
My preference is to put uncommon args that have defaults behind some sort of "name requirement" mechanism.
| factory: ResourceFactory[_T_co] = _Resource, | |
| *, | |
| factory: ResourceFactory[_T_co] = _Resource, |
| self, | ||
| ctx: Context, | ||
| path: str, | ||
| factory: ResourceFactory[_T_co] = _Resource, |
There was a problem hiding this comment.
We do not adjust factory or uid, but their default values are shared objects between instances: https://www.pythonmorsels.com/mutable-default-arguments/
(No changes necessary... just a thought to file away for a different debug day)
|
|
||
| T = TypeVar("T", bound=Resource) | ||
| _T = TypeVar("_T", bound=Resource) | ||
| _T_co = TypeVar("_T_co", bound=Resource, covariant=True) |
There was a problem hiding this comment.
A perfect use case!
Linking for my own reminder: https://stackoverflow.com/a/61737894/591574
There was a problem hiding this comment.
But is it necessary since we're cast()ing?
I think we might be able to get away with a invariant type.
| _T_co = TypeVar("_T_co", bound=Resource, covariant=True) | |
| _R = TypeVar("_R", bound=Resource) |
(R for factory R eturn value?)
| resource = self._factory(self._ctx, path, **result) | ||
| resource = cast(_T, resource) |
There was a problem hiding this comment.
| resource = self._factory(self._ctx, path, **result) | |
| resource = cast(_T, resource) | |
| resource: _T_co = self._factory(self._ctx, path, **result) | |
| resource = cast(_T, resource) |
| resource = self._factory(self._ctx, path, **result) | ||
| resource = cast(_T, resource) |
There was a problem hiding this comment.
From other repos, I've used an explicit type expectation before casting the value.
resource: _T_co = self._factory(self._ctx, path, **result)
resource = cast(_T, resource)While it feels redundant, it gives me confidence if we ever change ._factory() and its return type. This way, we know we are transferring from a _T_co to _T (and not an Any to a _T).
| resource = self._factory(self._ctx, path, **result) | ||
| resource = cast(_T, resource) |
There was a problem hiding this comment.
| resource = self._factory(self._ctx, path, **result) | |
| resource = cast(_T, resource) | |
| resource: _T_co = self._factory(self._ctx, path, **result) | |
| resource = cast(_T, resource) |
| resource = self._factory(self._ctx, path, **result) | ||
| resource = cast(_T, resource) |
There was a problem hiding this comment.
| resource = self._factory(self._ctx, path, **result) | |
| resource = cast(_T, resource) | |
| resource: _T_co = self._factory(self._ctx, path, **result) | |
| resource = cast(_T, resource) |
2 participants
No description provided.