Complying to #65 - reading parameters of different bulbs types #67
Conversation
pytradfri/device.py
Outdated
| raw_color = self.raw.get(ATTR_LIGHT_COLOR) | ||
| if raw_color is not None and len(raw_color) == 6: | ||
| return raw_color | ||
| (x, y) = self.xy_color |
There was a problem hiding this comment.
I'm not sure if I'm 100% happy with this.
This property used to be a direct 1:1 mapping with the underlying data from the API. I think that it should stay like that. If we want to offer smarter things on top, maybe we can offer it as a utility function?
There was a problem hiding this comment.
It's true for all hex_color, xy_color and kelvin_color. Maybe each of them need to have two flavours - "raw" and "best guess"? Maybe boolean parameter? Than it can default to one of flavours - which one you'd think would be better?
There was a problem hiding this comment.
Hm, they're properties, so it conflicts with parametrizing them. So two sets of properties maybe? [hex|xy|kelvin]_color_[raw|guess]? Or [hex|xy|kelvin]_color[_best_guess], or even [hex|xy|kelvin]_color[_raw]. I also don't have a good suffix for this new property - guess, best_guess, estimate? Neither captures exact behaviour, which is "If I have a value from API, return it, else estimate from other values from API".
There was a problem hiding this comment.
Are you not talking about something along the line of an inferred value?
https://en.wikipedia.org/wiki/Inference
There was a problem hiding this comment.
Yes, inference is the word, thanks! So how do you find a set of properties: [hex|xy|kelvin]_color_[raw|inferred]?
There was a problem hiding this comment.
But then we have inconsistency in kelvin_color, which is definitely inferred. And this change would be breaking because homeassistant checks bulb features by verifing if hex_color makes sense? So maybe it would be good to introduce breaking change, together with can_set_, to introduce more order? I'll then add can_set_ to this PR soon.
There was a problem hiding this comment.
Breaking with Home Assistant is fine as HASS uses pinned requirements. If we messed up with kelvin_color, let's fix it.
There was a problem hiding this comment.
OK, so which convention should we follow:
[hex|xy]_color[_raw]andkelvin_color[hex|xy]_color[_inferred]andkelvin_color_inferred[hex|xy]_color_[raw|inferred]andkelvin_color_inferred
? (My order from most to least preferred: 1, 3, 2)
There was a problem hiding this comment.
I don't follow your convention list. This is what I would prefer:
- I want, by default, property names to reflect what is returned from the API. xy_color comes directly from the returned dictionary. If we don't do this, the user can no longer trust if some data is there or that we're trying to come up with an answer, which might not fit their use case
- I want the property name to reflect if we're doing magic to make it work. So add
_inferred.
There was a problem hiding this comment.
OK, so what's you're describing is I guess 2) [hex|xy]_color[_inferred] and kelvin_color_inferred. which means 5 properties:
hex_color,xy_color,hex_color_inferred,xy_color_inferred,kelvin_color_inferred.
I'll change properties name that way.
|
I went with
|
|
Guess we're good to merge here? @balloob |
|
Merged and pushed to pypi as 3.0.1. Thanks @matemaciek for your contribution! |
No description provided.