dirigera icon indicating copy to clipboard operation
dirigera copied to clipboard

reload() method on devices not really a method?

Open nestorix1343 opened this issue 1 year ago • 1 comments

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?

nestorix1343 avatar Oct 28 '24 21:10 nestorix1343

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!

Leggin avatar Oct 31 '24 09:10 Leggin