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

Enable mypy attr defined #10923

Merged
merged 26 commits into from
Feb 24, 2022
Merged

Enable mypy attr defined #10923

merged 26 commits into from
Feb 24, 2022

Conversation

m-vdb
Copy link
Collaborator

@m-vdb m-vdb commented Feb 21, 2022

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@m-vdb m-vdb marked this pull request as ready for review February 22, 2022 10:25
@m-vdb m-vdb requested a review from a team February 22, 2022 10:25
@m-vdb m-vdb requested a review from a team as a code owner February 22, 2022 10:25
@m-vdb m-vdb requested review from dakshvar22, chandrikas and a team and removed request for a team, chandrikas and dakshvar22 February 22, 2022 10:25
rasa/shared/core/slots.py Outdated Show resolved Hide resolved
@m-vdb
Copy link
Collaborator Author

m-vdb commented Feb 22, 2022

@joejuzl I'm almost done on this PR and I need your help to figure the last type issue:

  • mypy doesn't like this code, here is the CI issue
  • here we define self._policy_schema_nodes as a List[SchemaNode] but really it is a "list of SchemaNode that represent a Policy"
  • I don't want to modify the code more than necessary here. One way to go about it would be to have a PolicySchemaNode that inherits SchemaNode but modifies uses to be Policy instead.

Do you have another idea?

@joejuzl
Copy link
Contributor

joejuzl commented Feb 24, 2022

Can you cast policy_node.uses to Policy? Or maybe even adding a isinstance or issubtype check in the generator comprehension would make mypy happy?
Also if there's no easy workaround, we always consider disabling a rule for one line, it's explicit at least.

@m-vdb
Copy link
Collaborator Author

m-vdb commented Feb 24, 2022

will give a try to those 👍🏻

@m-vdb
Copy link
Collaborator Author

m-vdb commented Feb 24, 2022

I went with the cast(Policy, ...), it's simple and explicit and I think it's better than to just disable the check

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.

🚀

@@ -162,7 +162,10 @@ class GCSPersistor(Persistor):
Fetches them when needed, instead of storing them on the local disk."""

def __init__(self, bucket_name: Text) -> None:
from google.cloud import storage
"""Initialise class with client and bucket."""
# there are no type hints in this repo for now
Copy link
Member

Choose a reason for hiding this comment

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

To double-check I understand this correctly, if there's 3rd party code without type hints without type hints, mypy will complain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes exactly. It's enabled by

rasa/setup.cfg

Line 43 in 348697b

disallow_untyped_calls = True

@m-vdb m-vdb merged commit 64a576a into main Feb 24, 2022
@m-vdb m-vdb deleted the enable-mypy-attr-defined branch February 24, 2022 09:47
@ancalita
Copy link
Member

ancalita commented Feb 24, 2022

@m-vdb I've just merged main to my mypy branch and getting some unrelated errors to the check I enabled (union-attr), full list here - is the fix to run make install to get latest mypy version?

rasa/utils/tensorflow/model_data.py:420: error: unused "type: ignore" comment
rasa/shared/core/trackers.py:86: error: Variable "typing_extensions.TypeAlias" is not valid as a type  [valid-type]
rasa/shared/core/trackers.py:86: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
rasa/shared/core/trackers.py:725: error: Variable "rasa.shared.core.trackers.EventTypeAlias" is not valid as a type  [valid-type]
rasa/shared/core/trackers.py:725: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
rasa/shared/core/trackers.py:729: error: Variable "rasa.shared.core.trackers.EventTypeAlias" is not valid as a type  [valid-type]
rasa/shared/core/trackers.py:729: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
rasa/core/lock_store.py:229: error: Library stubs not installed for "redis" (or incompatible with Python 3.8)  [import]
rasa/core/lock_store.py:229: note: Hint: "python3 -m pip install types-redis"
rasa/core/lock_store.py:229: note: (or run "mypy --install-types" to install all missing stub packages)
rasa/core/lock_store.py:229: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
rasa/shared/importers/importer.py:175: error: Too many arguments  [call-arg]
rasa/core/tracker_store.py:319: error: Library stubs not installed for "redis" (or incompatible with Python 3.8)  [import]
rasa/core/actions/action.py:863: error: EventTypeAlias? has no attribute "parse_data"  [attr-defined]
rasa/server.py:1368: error: unused "type: ignore" comment

@ancalita
Copy link
Member

@m-vdb edit: running make install cleared these errors - all good now. Just have more union-attr errors to fix than before actually 😅

@m-vdb
Copy link
Collaborator Author

m-vdb commented Feb 24, 2022

👍🏻 yeah, sorry I didn't mention that you needed to run make install. I'll share a message in #atom-squad

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.

Enable mypy attr-defined error code
3 participants