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

Auto update and expand _lights class variable in AdaptiveSwitch on access #562

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

th3w1zard1
Copy link
Collaborator

@th3w1zard1 th3w1zard1 commented Apr 8, 2023

It seemed arbitrary or unnecessarily picky when switch.py would used _expand_light_groups() before using _lights, now it just auto-expands on each access.

Code also uses switch._lights more sparingly and only once per method.

I also added # pylint: disable=protected-access to any function that accessed a class method outside of the class as that seemed to be what we've been going with.

@th3w1zard1 th3w1zard1 changed the title (Tiny Change) auto-expand groups and update when accessing _lights class variable in AdaptiveSwitch Auto update _lights class variable in AdaptiveSwitch on access (replaces _expand_light_groups) Apr 8, 2023
@th3w1zard1 th3w1zard1 changed the title Auto update _lights class variable in AdaptiveSwitch on access (replaces _expand_light_groups) Auto update and expand _lights class variable in AdaptiveSwitch on access Apr 8, 2023
@@ -838,6 +841,11 @@ def __init__(
data,
)

def __getattribute__(self, name):
Copy link
Owner

@basnijholt basnijholt Apr 10, 2023

Choose a reason for hiding this comment

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

what about creating a property on the switch:

@property
def all_lights(self):
    return self._expand_light_groups()

then use switch.all_lights everywhere?

Or maybe just lights (I not already in use in HA somewhere).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

brilliant.

Copy link
Collaborator Author

@th3w1zard1 th3w1zard1 Apr 10, 2023

Choose a reason for hiding this comment

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

We'd have to modify all the references with this method.

Copy link
Owner

Choose a reason for hiding this comment

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

I see your reasoning :)

However, a custom __getattribute__ might be hard to overlook when debugging because it is not super obvious what happens from just looking at the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not looking forward to modifying the other PRs. Perhaps we merge this one last?

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.

2 participants