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

Cache descriptors on first access #1701

Merged
merged 11 commits into from
Jan 30, 2023
Merged

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Jan 29, 2023

The descriptors accessed through sensors(), settings(), and actions() are now cached on the first use to avoid unnecessary I/O.

follow up on PR #1592 which got merge conflicts and is closed.

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2023

Codecov Report

Merging #1701 (2ad5f53) into master (6814f32) will increase coverage by 0.08%.
The diff coverage is 86.95%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #1701      +/-   ##
==========================================
+ Coverage   81.66%   81.74%   +0.08%     
==========================================
  Files         191      191              
  Lines       17913    17934      +21     
  Branches     3843     3845       +2     
==========================================
+ Hits        14628    14660      +32     
+ Misses       2997     2986      -11     
  Partials      288      288              
Impacted Files Coverage Δ
miio/device.py 85.38% <86.95%> (+9.38%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@starkillerOG
Copy link
Contributor Author

@rytilahti could you review/merge this?

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.

Looks fairly ready to go to me, please see the comments for some minor issues.

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

@rytilahti the tests are actually succesfull, there is an error uploading the codecov....
That is unrelated to this PR.

mocker.patch("miio.Device._sensor_descriptors_from_status", return_value={})
mocker.patch("miio.Device._setting_descriptors_from_status", return_value={})
mocker.patch("miio.Device._action_descriptors", return_value={})
for _i in range(5):
Copy link
Owner

Choose a reason for hiding this comment

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

Just for the future, the proper way to do this is to use plain _ here. No need to fix that now though.

@starkillerOG
Copy link
Contributor Author

@rytilahti all tests have passed

@rytilahti rytilahti changed the title Cache descriptors only once Cache descriptors on first access Jan 30, 2023
@rytilahti rytilahti merged commit 7e7834a into rytilahti:master Jan 30, 2023
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