Skip to content

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

@starkillerOG
Copy link
Contributor Author

@rytilahti processed the feedback

@starkillerOG
Copy link
Contributor Author

@rytilahti could you merge?

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.

3 participants