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 gateway get_devices_from_dict #1456

Merged
merged 4 commits into from
Jul 15, 2022

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Jul 14, 2022

Improve the get_devices_from_dict function of the gateway:
Now the device_id from direct communication is checked against the device_id received from the cloud.
Besides the already presend mac matching.
The _did is removed from the gateway since it is the same as the device_id which was already presend.

@starkillerOG
Copy link
Contributor Author

@rytilahti could you review and merge?
(this was discovered during my work on the push server, but actually has nothing to do with the push server so I split it out).

miio/gateway/gateway.py Outdated Show resolved Hide resolved
miio/gateway/gateway.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2022

Codecov Report

Merging #1456 (482a933) into master (4dd3cd8) will decrease coverage by 0.03%.
The diff coverage is 6.66%.

@@            Coverage Diff             @@
##           master    #1456      +/-   ##
==========================================
- Coverage   83.35%   83.32%   -0.04%     
==========================================
  Files         139      139              
  Lines       13670    13675       +5     
  Branches     3248     3249       +1     
==========================================
  Hits        11395    11395              
- Misses       2057     2062       +5     
  Partials      218      218              
Impacted Files Coverage Δ
miio/gateway/gateway.py 46.19% <6.66%> (-1.40%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@starkillerOG
Copy link
Contributor Author

@rytilahti thanks for the review, I have processed your feedback.
Could you have a look again?

@starkillerOG
Copy link
Contributor Author

I think the test failure is unrelated

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

LGTM, please add a note to description on why/how this improves it, then this is ready to go.

@starkillerOG
Copy link
Contributor Author

@rytilahti I updated the description of the PR, ready to go.

@rytilahti rytilahti merged commit 8402039 into rytilahti:master Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants