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 mypy complaints #137

Merged
merged 11 commits into from
Nov 3, 2023
Merged

Conversation

Lash-L
Copy link
Collaborator

@Lash-L Lash-L commented Nov 1, 2023

With py.typed enabled there are a lot of mypy complaints on the component side using the library.

This makes a few changes such as making some variables that we require/ know we will have into empty versions of themselves before we get them, i.e. Status.

Exporting some commonly accessed values like the name of the dock error

In the old system it was

props.status.dock_error.name But status could be none and dock_error could be None and that was two additional None checks that had to be done in the component side - this simplifies things.

@humbertogontijo
Copy link
Owner

humbertogontijo commented Nov 1, 2023

I'm ok with the new fields within Status class

Something else is wrong with typing. RoborockBase.from_dict can return None

@Lash-L
Copy link
Collaborator Author

Lash-L commented Nov 2, 2023

I'm ok with the new fields within Status class

Something else is wrong with typing. RoborockBase.from_dict can return None

Hmm

Should it raise when from dict is passed a non dict and returns none? Or create an empty version of the class.

I see what you mean now. What's you opinion on best typing there?

@humbertogontijo
Copy link
Owner

Raising an exception is a good way to type it. But at the same time it could have a big impact on core and hacs implementation

@Lash-L
Copy link
Collaborator Author

Lash-L commented Nov 2, 2023

Hows that? A few guaranteed types - others optional None

roborock/api.py Outdated Show resolved Hide resolved
roborock/api.py Outdated Show resolved Hide resolved
@Lash-L
Copy link
Collaborator Author

Lash-L commented Nov 2, 2023

I'll fix lint commit in merge

@Lash-L Lash-L merged commit 752e320 into humbertogontijo:main Nov 3, 2023
5 of 6 checks passed
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.

2 participants