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

reload() method on devices not really a method? #103

Open
nestorix1343 opened this issue Oct 28, 2024 · 1 comment
Open

reload() method on devices not really a method? #103

nestorix1343 opened this issue Oct 28, 2024 · 1 comment

Comments

@nestorix1343
Copy link

The devices classes (light, outlet, and so on) have a reload() method defined.

The fact that this is a method for a class object and the name "reload" suggests (at least that is what I think) to reload the instance to the current setting/status of the Dirigera hub.

However, the reload() method just returns a new class instance which is not logical in my point of view.

So if you have a variable containing a Light class, for instance:

some_light: Light = hub.get_light_by_id("some-id")
print(some_light.light_level)  # prints for instance 12
some_light.set_light_level(34)
some_light.reload()
print(some_light.light_level)  # prints still 12, expected 34 

One would expect that after the reload, some_light has its light level set to 34. This is not the case. This only works if you do:

some_light = some_light.reload()

However, that does not really look how classes and instances of classes should work.

Or am I completely wrong?

@Leggin
Copy link
Owner

Leggin commented Oct 31, 2024

Hey, you’re absolutely right; it does feel unintuitive to use the reload() method this way. The current approach of returning a new instance rather than updating the existing one was a decision made primarily to avoid directly reassigning all fields on the current instance. Manually updating each field is both error-prone and would lead to duplicated code across different device classes.

An alternative could be to update reload() to refresh the existing instance's attributes in place, which would align better with expected behavior. Another option might be renaming the method to something like fetch_updated_instance() to clarify that it returns a new instance with updated values rather than modifying the existing one.

Thanks for pointing this out—definitely something to consider improving!

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

No branches or pull requests

2 participants