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 #88

Merged
merged 2 commits into from
Mar 12, 2021
Merged

Enable mypy #88

merged 2 commits into from
Mar 12, 2021

Conversation

KapJI
Copy link
Collaborator

@KapJI KapJI commented Mar 11, 2021

Add it to pre-commit config.

Unfortunately, homeassistant, voluptuous and glocaltokens don't distribute typing information.

I'll try to enable it for homeassistant.
@leikoilja can you enable this for glocaltokens? Should be one line in setup.py.
https://www.python.org/dev/peps/pep-0561/

@KapJI KapJI requested a review from leikoilja March 11, 2021 20:02
@KapJI KapJI mentioned this pull request Mar 11, 2021
@@ -1,7 +1,9 @@
"""Models for Google Home"""

from __future__ import annotations
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for using forward declared types in type hints: https://www.python.org/dev/peps/pep-0563/

Copy link
Owner

@leikoilja leikoilja Mar 12, 2021

Choose a reason for hiding this comment

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

hmmmm, my knowledge is quite limited on __future__ and annotations, so dont really follow what it's doing 🤔 It is postponing annotations to prevent from runtime executed annotations. But why? Do you understand showing annotations at runtime when function/variable is called is a problem? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically this allows you to use types before they're defined.
For example here it is:

def get_next_alarm(self) -> Optional[GoogleHomeAlarm]:

Otherwise you need to use string:

def get_next_alarm(self) -> Optional["GoogleHomeAlarm"]:

Should be enabled by default in Python 3.10.

Copy link
Owner

Choose a reason for hiding this comment

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

uuu that't nice, thanks, i hope i get it a bit better now

@KapJI
Copy link
Collaborator Author

KapJI commented Mar 12, 2021

Feel free to add type hints to more parts after you merge this 🙂

@KapJI
Copy link
Collaborator Author

KapJI commented Mar 12, 2021

@leikoilja can you take a look? 🙂

Copy link
Owner

@leikoilja leikoilja left a comment

Choose a reason for hiding this comment

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

In general looks really good to me! Thanks for making it! I have a few conceptual questions, but those are mostly around my limited understanding of a topic :) If you could help clearing those up, i'd be happy

Comment on lines +21 to +28
[mypy-glocaltokens.*]
ignore_missing_imports = True

[mypy-homeassistant.*]
ignore_missing_imports = True

[mypy-voluptuous.*]
ignore_missing_imports = True
Copy link
Owner

Choose a reason for hiding this comment

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

Why is it needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These packages don't distrubute typing information (don't have py.typed marker) and mypy will complain that it can't import these packages.

Copy link
Owner

Choose a reason for hiding this comment

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

Got it, thank you!
Check glocaltokens open PR, i think it's ready to go. I have added py.typed as requested :)

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, with it we can remove glocaltokens from here and type check its usages :)

@KapJI KapJI merged commit b51afaf into leikoilja:master Mar 12, 2021
@KapJI KapJI deleted the mypy branch March 12, 2021 14:04
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.

None yet

2 participants