Update decorators #1586
Update decorators #1586
Conversation
starkillerOG
commented
Nov 9, 2022
starkillerOG
commented
- set the id to the qualified_name instead of only the function name (in order to prevent unique_id collisions in HomeAssistant for example with DNDStatus.start and CleaningDetails.start)
- fix sensor unit beeing set to "" instead of None (HomeAssistant default)
- remove setter input from setting decorator, since it will not work to put in a function directly in the decorator since the class object has not yet been created. Therefore always the setter_name schould be used and the binding of the setter methods schould take place when the class object gets created.
|
@rytilahti this is split out from #1543 |
Codecov Report
@@ Coverage Diff @@
## master #1586 +/- ##
=======================================
Coverage 80.88% 80.88%
=======================================
Files 156 156
Lines 15255 15257 +2
Branches 3283 3283
=======================================
+ Hits 12339 12341 +2
Misses 2667 2667
Partials 249 249
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| if setter_name is None: | ||
| raise Exception("Setter_name needs to be defined") |
There was a problem hiding this comment.
Why is setter parameter being removed?
There was a problem hiding this comment.
Since it will not work to put in a function directly in the decorator since the class object has not yet been created. Therefore always the setter_name schould be used and the binding of the setter methods schould take place when the class object gets created.
I tried to use the setter parameter in my code directly in the decorator, but I could not get it to work (I think it will never work).
There was a problem hiding this comment.
One way would be to give it a reference to a callable (like DeviceClass.set_something), and binding that to the instance object much like what is done for the setter_name one, no?`.
But that is not important at the moment, let's however keep the code in place just in case for the time being.
There was a problem hiding this comment.
I don't think that works since how would you get the instance object from the DeviceClass.set_something callable. But I am quite new to all these decorators, still figuring it all out a bit.
Learning a lot from your coding skills!
Schould I just keep the setter parameter and add a Exception that it is not yet implemented instead?
There was a problem hiding this comment.
Yes, just add an exception there. I'm also learning, so it's all good :-)
I think something like this could work for the binding, but I haven't tried it out:
from functools import partial
method = partial(setter, self) # now the `self` is passed as the first parameter to the method calls
There was a problem hiding this comment.
I made the exception in a seperate PR.
It's better to keep one type of change in one PR, this also makes it easier to write more clear PR titles than "update X". Could you move this into a separate PR? I'm wondering if we should remove the |
|
@rytilahti I think we schould leave the unit for sensors since I think it is very important for almost all sensors and this streches the importance of the unit. |
Split out from #1586 set the id to the qualified_name instead of only the function name (in order to prevent unique_id collisions in HomeAssistant for example with DNDStatus.start and CleaningDetails.start)
Fair enough, let's keep it for now, especially as we may want to create a standardized set of units (in a form of enum?) to make the unit information consistent across the integrations. |
Split out from: #1586 fix sensor unit beeing set to "" instead of None (HomeAssistant default)
Split out from #1586 warn for setter input from setting decorator, since it will not work to put in a function directly in the decorator since the class object has not yet been created. Therefore always the setter_name schould be used and the binding of the setter methods schould take place when the class object gets created. Co-authored-by: Teemu R. <[email protected]>
