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

Fix and enable mypy return check #9109

Merged
merged 47 commits into from
Jul 15, 2021
Merged

Fix and enable mypy return check #9109

merged 47 commits into from
Jul 15, 2021

Conversation

joejuzl
Copy link
Contributor

@joejuzl joejuzl commented Jul 12, 2021

Proposed changes:
Fixes #8242

@joejuzl joejuzl requested a review from a team as a code owner July 12, 2021 13:49
@joejuzl joejuzl requested review from a team and koernerfelicia and removed request for a team and koernerfelicia July 12, 2021 13:49
Copy link
Member

@ancalita ancalita left a 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
Copy link
Member

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/actions/action.py Show resolved Hide resolved
rasa/core/actions/forms.py Show resolved Hide resolved
@@ -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]:
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please

@wochinge wochinge requested a review from ancalita July 14, 2021 15:30
poetry.lock Show resolved Hide resolved
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

🎉

@wochinge wochinge enabled auto-merge July 15, 2021 16:21
@wochinge wochinge merged commit 08bf76b into main Jul 15, 2021
@wochinge wochinge deleted the 8242-mypy_return_issues branch July 15, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypy: Fix return issues
3 participants