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

Add device_id property to Device class #1384

Merged
merged 8 commits into from
Apr 4, 2022

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Apr 4, 2022

This PR adds new device_id property to Device class.
The device_id is part of the handshake reply and thus available also on devices with unknown tokens (i.e., it can be useful for use cases where the token is extract from the cloud or other external sources).

This is also needed for push server implementation: #1288

@starkillerOG
Copy link
Contributor Author

@rytilahti could you merge this?

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #1384 (b929d79) into master (7269b15) will decrease coverage by 0.01%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master    #1384      +/-   ##
==========================================
- Coverage   84.17%   84.15%   -0.02%     
==========================================
  Files         133      133              
  Lines       13319    13324       +5     
  Branches     1483     1484       +1     
==========================================
+ Hits        11211    11213       +2     
- Misses       1897     1900       +3     
  Partials      211      211              
Impacted Files Coverage Δ
miio/device.py 53.89% <40.00%> (-0.47%) ⬇️

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

miio/miioprotocol.py Outdated Show resolved Hide resolved
miio/device.py Outdated Show resolved Hide resolved
@starkillerOG
Copy link
Contributor Author

@rytilahti processed the feedback

@starkillerOG
Copy link
Contributor Author

@rytilahti could you merge?

miio/device.py Outdated Show resolved Hide resolved
miio/device.py Outdated Show resolved Hide resolved
miio/device.py Outdated Show resolved Hide resolved
starkillerOG and others added 4 commits April 4, 2022 23:45
Co-authored-by: Teemu R. <tpr@iki.fi>
Co-authored-by: Teemu R. <tpr@iki.fi>
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.

This will cause I/O if the device hasn't been initialize prior trying to access the property, but I think the pros outweigh the cons. If needed, this can be revisited in some later point.

Thanks @starkillerOG, this is good to go as soon as some tests are added to check the both code paths! 👍

@starkillerOG
Copy link
Contributor Author

@rytilahti I just added a test, could you check if that is what you ment.

@rytilahti rytilahti changed the title Add device_id property Add device_id property to Device class Apr 4, 2022
@rytilahti rytilahti merged commit fa15166 into rytilahti:master Apr 4, 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.

None yet

3 participants