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

Make embedded statuses directly accessible attributes #1542

Closed
wants to merge 6 commits into from

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Sep 30, 2022

Consider this test script with a Roborock vacuum:

from miio import RoborockVacuum
r = RoborockVacuum(ip = "192.168.1.IP", token = "TokenTokenToken")
status = r.status()

print(status["CleaningSummary:count"])
Will give
TypeError: 'VacuumStatus' object is not subscriptable

print(status.CleaningSummary:count)
will give
SyntaxError: invalid syntax
This is a invalid syntax since variable names can only contain (A-Z, a-z), (0-9), and (_), see https://realpython.com/python-variables/#variable-names

Therefore overwriding getattribute will not work as intended.
Alright so it does work when using getattr(status, "CleaningSummary:count") which is used inside HomeAssistant for conveniance. So lets keep it.

This PR will make the embeded classes a attribute of the main class, so you can acess the embedded classes using:

print(status.CleaningSummary.count)
Which will work

(This is still needed since using normal python scripts there needs to be an easy way to acces the embeded containers)

@starkillerOG
Copy link
Contributor Author

starkillerOG commented Sep 30, 2022

@rytilahti I don't know why mypy is complaining about the ._sensors etc.
But in any case that is unrelated to this PR.

@starkillerOG
Copy link
Contributor Author

@rytilahti could you have a look at this PR, it is very small and simple.

@rytilahti
Copy link
Owner

Yeah, I was thinking about the naming and I knew it would not be a valid identifier for direct accesses. So, I'm wondering if we should just use double-underscore instead of : as a separator, this would also allow accesses without going through the subcontainer name.

@@ -119,6 +119,7 @@ def embed(self, other: "DeviceStatus"):
other_name = str(other.__class__.__name__)

self._embedded[other_name] = other
setattr(self, other_name, other)
Copy link
Owner

Choose a reason for hiding this comment

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

We can add this (and update the docstring to inform anyone reading the docs about the fact), and also change the separator to __ instead of :, in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No __ can not be used as seperator, I tried it but very bad things start happening.
Python uses __ for python variables like __main__, __name__ and even worse __getattribute__ itself is also an attribute to the class, so you get into an infinite loop of __getattribute__ getting itself using another __getattribute__ and you crash the program.

Copy link
Owner

@rytilahti rytilahti Oct 8, 2022

Choose a reason for hiding this comment

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

I don't think having a double undescore should be a problem as a separator in the middle of the name, as far as I know the dunder methods need to both start and end with __.

What sort of bad things were you experiencing?

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Merging #1542 (a9ea661) into master (f4abd59) will increase coverage by 0.25%.
The diff coverage is 100.00%.

❗ Current head a9ea661 differs from pull request most recent head 49ef800. Consider uploading reports for the commit 49ef800 to get more accurate results

@@            Coverage Diff             @@
##           master    #1542      +/-   ##
==========================================
+ Coverage   80.82%   81.08%   +0.25%     
==========================================
  Files         151      153       +2     
  Lines       14798    15017     +219     
  Branches     1696     1713      +17     
==========================================
+ Hits        11960    12176     +216     
- Misses       2591     2594       +3     
  Partials      247      247              
Impacted Files Coverage Δ
miio/devicestatus.py 88.37% <100.00%> (+0.13%) ⬆️
miio/integrations/vacuum/mijia/__init__.py 100.00% <0.00%> (ø)
miio/integrations/vacuum/mijia/pro2vacuum.py 98.14% <0.00%> (ø)
...integrations/vacuum/mijia/tests/test_pro2vacuum.py 100.00% <0.00%> (ø)

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

@starkillerOG
Copy link
Contributor Author

@rytilahti ready for another review/merge

@rytilahti rytilahti changed the title fix retrieving embedded status Make embedded statuses directly accessible attributes Oct 18, 2022
@starkillerOG
Copy link
Contributor Author

Closing this as it has been resolved by #1573

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

Successfully merging this pull request may close these issues.

None yet

3 participants