-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Fix and enable mypy return check #9109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look into most of the research-owned files, you might need to loop in somebody from Research for a more in-depth review 😬
@@ -165,6 +165,7 @@ def run_core_training( | |||
rasa.utils.common.run_in_loop( | |||
do_compare_training(args, story_file, additional_arguments) | |||
) | |||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very sure what do_compare_training
does, but it also returns None
, is there any reason why it shouldn't be return rasa.utils.common.run_in_loop(...)
?
rasa/core/policies/sklearn_policy.py
Outdated
@@ -338,7 +338,7 @@ def persist(self, path: Union[Text, Path]) -> None: | |||
@classmethod | |||
def load( | |||
cls, path: Union[Text, Path], should_finetune: bool = False, **kwargs: Any | |||
) -> Policy: | |||
) -> Optional[Policy]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't do this @joejuzl as this goes against the Policy
interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha yes true. TEDPolicy.load
was recently changed to have return cls()
. We can do that here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Proposed changes:
Fixes #8242