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

Improve vehicle init and some bugfixes #116

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Improve vehicle init and some bugfixes #116

merged 1 commit into from
Dec 7, 2021

Conversation

DurgNomis-drol
Copy link
Owner

@DurgNomis-drol DurgNomis-drol commented Dec 7, 2021

Should fix #111 and fix missing await in the last PR #110

This is not a complete refactor of the vehicle init, but a start

Copy link
Collaborator

@joro75 joro75 left a comment

Choose a reason for hiding this comment

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

Ok. These changes will certainly help.
There are however still two questions, I would like to discuss. See my other comments.

@joro75
Copy link
Collaborator

joro75 commented Dec 7, 2021

@DurgNomis-drol Question 1:
Why wass the problem with the missing async in the return statement not detected by the unittests?
I'm not into the details of the pylint or mypy, but I now understand that these checkers are not able to check if async/await is used correctly in all cases?
Can we extend the unittests to include an actual test of the code in controller.py to catch this (and other errors) in an earlier stage?

@joro75
Copy link
Collaborator

joro75 commented Dec 7, 2021

@DurgNomis-drol Question 2:
Now more members of Vehicle are being checked if they are assigned/existing before they are being interrogated. This doesn't sound very pythonic. Wouldn't it be possible to use a customized Dict-type that just returns a default empty Dict if itself, or a member of it is not present? I do realize that this will be an extension on top of the basic python Dict type.
I'm still thinking on how we can find a good solution for the problem that depending on the vehicle for which the data is requested, some (data-)members can be present or not.

I do have an idea, but I will have to try and investigate it a little bit more in detail.

@joro75 joro75 merged commit 39dbe72 into master Dec 7, 2021
@joro75 joro75 deleted the bugfix branch December 7, 2021 20:54
@DurgNomis-drol
Copy link
Owner Author

Thanks for reviewing!!

Question 1: The previous PR was created in a different dev environment then i normally use and it is indeed strange that i was not caught back then. This PR was made on my normal setup and there it caught as an error by pylint, so there is something wrong with the first setup I used and the one that is running on github CI, but i am no sure why.

And yes we should extend unitests to cover controller.py as well.

This link may explain why it didn't catch the error: https://stackoverflow.com/questions/64894694/which-python-static-checker-can-catch-forgotten-await-problems though both functions are typed
All the more reason to get this library typed. 😄

Question 2: I am not sure i understand what you mean by using a typed dict for class members? Vehicle init should probably be be rewritten so that we reuse the object when updating instead of creating a new one every time there is new data. Like this:
Init vehicle and then every time we want to update a specific member of the object, we call like this: vehicle.update_hvac() and then this only updates the hvac part. Does this make sense to do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connected services is falsely reported as not setup
3 participants