Switch to mypy #247
Switch to mypy #247
Conversation
|
Current status:
(also ran it on cc @tmbo @wochinge FYI. There might be some thing we'd want to fix before 2.0 final release, but maybe non-blocking |
| forms: List[Union[Text, Dict]] | ||
|
|
||
|
|
||
| class ActionCall(TypedDict): |
There was a problem hiding this comment.
these type definitions need proper review + docstrings
| @@ -213,17 +214,18 @@ def entity_is_desired( | |||
| """ | |||
|
|
|||
| # slot name is equal to the entity type | |||
| other_slot_equals_entity = other_slot == other_slot_mapping.get("entity") | |||
| entity_type = other_slot_mapping.get("entity") | |||
| other_slot_equals_entity = other_slot == entity_type | |||
There was a problem hiding this comment.
this is part of a small refactoring done in 4dec28a
| @@ -567,7 +578,10 @@ async def _validate_if_required( | |||
| if utils.is_coroutine_action(self.validate): | |||
| return await self.validate(dispatcher, tracker, domain) | |||
| else: | |||
| return self.validate(dispatcher, tracker, domain) | |||
| # see https://github.com/python/mypy/issues/5206 | |||
There was a problem hiding this comment.
if we have to do this a lot in rasa, we might consider contributing to mypy to support useful type guards (I honestly don't know how hard it would be, but it could be worth it)
There was a problem hiding this comment.
yes this sounds like a good idea 👍 maybe something for someone from the team to contribute?
There was a problem hiding this comment.
yes definitely. I will keep this as a note for now, and if this comes back I'll mention it (in backend eng meeting for instance)
| from typing_extensions import TypedDict | ||
|
|
||
|
|
||
| class TrackerState(TypedDict): |
There was a problem hiding this comment.
should we rather use pythons new data classes? (and a backport for 3.6 https://pypi.org/project/dataclasses/)
There was a problem hiding this comment.
I think that could be better indeed, but I do not know enough about the specifics of the tracker state / action call:
- would renaming the method
from_dict()tofrom_dataclass()be acceptable? - is the type definition I made here valid? (I marked some elements required, before they were optional)
There was a problem hiding this comment.
after thinking a bit more about it, I'm not sure that we would benefit from the dataclass here (vs keeping the TypedDict). And if we wanted to do this it might change the public API of the SDK. For instance, instead of calling custom action with plain dicts, we would call them with dataclasses. And I'm not sure it's worth the trouble. WDYT?
There was a problem hiding this comment.
ok that sounds reasonable, let's stick with the typeddicts 👍
| from typing_extensions import TypedDict | ||
|
|
||
|
|
||
| class TrackerState(TypedDict): |
There was a problem hiding this comment.
ok that sounds reasonable, let's stick with the typeddicts 👍
Proposed changes:
pytypeit took 35s; now it seems to take 2s withmypyStatus (please check what you already did):
black(please check Readme for instructions)