Retrofit assert_type for Aggregate and Callable#935
Retrofit assert_type for Aggregate and Callable#935ZipFile merged 3 commits intoets-labs:developfrom
Conversation
leonarduschen
left a comment
leonarduschen
left a comment
There was a problem hiding this comment.
I've only worked on aggregate.py and callable.py, but found some areas that need quick-fixings.
I think it'd make more sense to include these quick-fixes in the same PR, as the tests are supposed to check exactly the behaviors these quick-fixes are supposed to affect.
@ZipFile let me know what you think.
tests/typing/aggregate.py
Outdated
|
|
||
|
|
||
| provider1_new_non_string_keys: providers.Aggregate[str] = providers.Aggregate( | ||
| # TODO: Change providers.Aggregate to accept Mapping? Then remove explicit typing here |
There was a problem hiding this comment.
Current stub:
class Aggregate(Provider[T]):
def __init__(
self,
provider_dict: Optional[_Dict[Any, Provider[T]]] = None,
**provider_kwargs: Provider[T],
): ...I think we should accept collections.abc.Mapping here
class Aggregate(Provider[T]):
def __init__(
self,
provider_dict: Optional[Mapping[Any, Provider[T]]] = None,
**provider_kwargs: Provider[T],
): ...Because Dict is invariant in both key and value, so providers.Aggregate cannot infer its own typevar without explicit typing
provider1_new_non_string_keys = providers.Aggregate(
{Cat: providers.Object("str")},
) # This raises mypy error
tests/typing/callable.py
Outdated
| args4 = provider4.args | ||
| kwargs4 = provider4.kwargs | ||
| assert_type(args4, Tuple[Any]) | ||
| # TODO: Change Callable.kwargs to Dict[str, Any]? Then adjust test back to Dict[str, Any] |
There was a problem hiding this comment.
The stub actually declares provider4.kwargs to return Dict[Any, Any], not Dict[str, Any] like the previous test probably intended to check.
But I think we can safely change it to return Dict[str, Any] in the stub.
tests/typing/callable.py
Outdated
| attr_getter5 = provider5.provided.attr | ||
| item_getter5 = provider5.provided["item"] | ||
| method_caller5 = provider5.provided.method.call(123, arg=324) | ||
| # TODO: Remove explicit typing of Provider.provided return type |
There was a problem hiding this comment.
The stub is this
@property
def provided(self) -> ProvidedInstance[T]: ...But ProvidedInstance is not declared to be generic
class ProvidedInstance(Provider, ProvidedInstanceFluentInterface):
def __init__(self, provides: Optional[Provider] = None) -> None: ...I think we can make the quick fix to
@property
def provided(self) -> ProvidedInstance: ...| # Test 10: to check the .provides | ||
| provider10 = providers.Callable(Cat) | ||
| provides10: Optional[Callable[..., Cat]] = provider10.provides | ||
| assert provides10 is Cat |
There was a problem hiding this comment.
I think these assert statements are not doing anything
tests/typing/callable.py
Outdated
|
|
||
| # Test 12: to check string imports | ||
| provider12: providers.Callable[Dict[Any, Any]] = providers.Callable("builtins.dict") | ||
| # TODO: Use T_Any as Callable typevar? Then remove type-ignore |
There was a problem hiding this comment.
Can't infer typevar without explicit typing, mypy raises error here, but we can change it to typevar with default Any to fix it
ZipFile
left a comment
ZipFile
left a comment
There was a problem hiding this comment.
Hello! Sorry for the late review. I've addressed issues you've mentioned. Can you rebase the branch with latest develop and remove TODOs?
|
Sure! I'll do it sometime this week. |
leonarduschen
force-pushed
the
retrofit-assert-type
branch
from
143b86d to
35ecd2c
Compare
3 participants
Partially addresses #934