Lamp value range validation without mistakes #140

Lakitna · 2018-01-19T16:31:34Z

I've made a mistake validating what I did in #139.

Rebase fork

Also: Fixed references to Hue and Sat that where flipped.

Lakitna · 2018-01-19T16:33:30Z

There are some lamp values that can be set individually, but others can't.

XY: When setting X you must also set Y, otherwise Y=0
HS: When setting Hue you must also set Saturation, otherwise Saturation=0

When setting either of these 4 values as None they will become 0.

coveralls · 2018-01-19T16:34:36Z

Coverage decreased (-0.1%) to 60.415% when pulling 8710707 on Lakitna:Lamp-value-range-validation into 807ef60 on ggravlingen:master.

ggravlingen · 2018-01-19T17:10:05Z

pytradfri/const.py

@@ -56,8 +56,8 @@
 ATTR_LIGHT_COLOR_HEX = "5706"  # string representing a value in hex
 ATTR_LIGHT_COLOR_X = "5709"
 ATTR_LIGHT_COLOR_Y = "5710"
-ATTR_LIGHT_COLOR_HUE = "5707"
-ATTR_LIGHT_COLOR_SATURATION = "5708"
+ATTR_LIGHT_COLOR_HUE = "5708"


Are you certain about this? Looking at the constants in the Android app, it should be the other way around: https://hastebin.com/zofoqucibe.php

You are right, I've changed a bit too much here. The other places I flipped hue with sat are correct though.

ggravlingen · 2018-01-20T08:06:01Z

@Lakitna can you please also add a few tests for the stuff you've added?

coveralls · 2018-01-20T12:36:31Z

Coverage decreased (-0.1%) to 60.415% when pulling 9d83e9c on Lakitna:Lamp-value-range-validation into 807ef60 on ggravlingen:master.

houndci-bot · 2018-01-20T12:59:05Z

tests/test_device.py

+        dev.light_control._value_validate(9, rnge)
+    with pytest.raises(ValueError) as e_info:
+        dev.light_control._value_validate(-1, rnge)
+    with pytest.raises(ValueError) as e_info:


local variable 'e_info' is assigned to but never used

coveralls · 2018-01-20T13:03:29Z

Coverage increased (+0.08%) to 60.622% when pulling 6dee151 on Lakitna:Lamp-value-range-validation into 807ef60 on ggravlingen:master.

Added default `None` for brightness in `set_hsb()`. This is the only value that's not required here.

coveralls · 2018-01-20T13:32:36Z

Coverage increased (+1.01%) to 61.554% when pulling 7f73237 on Lakitna:Lamp-value-range-validation into 807ef60 on ggravlingen:master.

While I'm at it... These functions are very similar anyways.

coveralls · 2018-01-20T13:43:28Z

Coverage increased (+2.8%) to 63.316% when pulling 8d6b421 on Lakitna:Lamp-value-range-validation into 807ef60 on ggravlingen:master.

Trying to minimize repetitive tests of the same function.

coveralls · 2018-01-20T13:48:25Z

Coverage increased (+2.8%) to 63.316% when pulling bf92e91 on Lakitna:Lamp-value-range-validation into 807ef60 on ggravlingen:master.

Lakitna · 2018-01-20T13:55:02Z

tests/test_device.py

+
+    command = dev.light_control.set_hex_color("RandomString")
+    data = command.data[ATTR_LIGHT_CONTROL][0]
+    assert len(data) is 1


Is it a problem that every string is accepted in set_hex_color? Perhaps we should limit the string length to 6, that could prevent accidental network overload.

Not needed now, let's do it in another PR later on.

coveralls · 2018-01-20T13:55:19Z

Coverage increased (+2.9%) to 63.42% when pulling 033c93b on Lakitna:Lamp-value-range-validation into 807ef60 on ggravlingen:master.

ggravlingen · 2018-01-20T15:41:31Z

@Lakitna I guess this can be merged now?

Lakitna · 2018-01-20T16:46:49Z

Yes, if you agree it can be merged :)

ggravlingen · 2018-01-20T17:03:32Z

Thank you @Lakitna! Adding those tests is very appreciated. I've pushed this to pypi now.

Lakitna and others added 3 commits January 17, 2018 14:41

Merge pull request #1 from ggravlingen/master

abe8296

Rebase fork

Allow None as a value

6b518b9

Allow setting HS without setting B

8710707

Also: Fixed references to Hue and Sat that where flipped.

ggravlingen reviewed Jan 19, 2018

View reviewed changes

ggravlingen changed the title ~~Lamp value range validation without mistakes~~ WIP: Lamp value range validation without mistakes Jan 20, 2018

Fixed hue and sat endpoints

9d83e9c

Added tester for _value_validate()

a55135f

houndci-bot reviewed Jan 20, 2018

View reviewed changes

Fixed Flake8 & Hound

6dee151

Added test for set_hsb

7f73237

Added default `None` for brightness in `set_hsb()`. This is the only value that's not required here.

Added tests for set_xy_color, set_color_temp, set_dimmer, and set_state

8d6b421

While I'm at it... These functions are very similar anyways.

Finding out how Coverall likes things

bf92e91

Trying to minimize repetitive tests of the same function.

Added test for set_hex_color

033c93b

Lakitna commented Jan 20, 2018

View reviewed changes

ggravlingen changed the title ~~WIP: Lamp value range validation without mistakes~~ Lamp value range validation without mistakes Jan 20, 2018

ggravlingen merged commit 4a8e2e6 into home-assistant-libs:master Jan 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lamp value range validation without mistakes #140

Lamp value range validation without mistakes #140

Lakitna commented Jan 19, 2018

Lakitna commented Jan 19, 2018

coveralls commented Jan 19, 2018 •

edited

Loading

ggravlingen Jan 19, 2018

Lakitna Jan 20, 2018

ggravlingen commented Jan 20, 2018

coveralls commented Jan 20, 2018 •

edited

Loading

houndci-bot Jan 20, 2018

coveralls commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

Lakitna Jan 20, 2018 •

edited

Loading

ggravlingen Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

ggravlingen commented Jan 20, 2018

Lakitna commented Jan 20, 2018

ggravlingen commented Jan 20, 2018

Lamp value range validation without mistakes #140

Lamp value range validation without mistakes #140

Conversation

Lakitna commented Jan 19, 2018

Lakitna commented Jan 19, 2018

coveralls commented Jan 19, 2018 • edited Loading

ggravlingen Jan 19, 2018

Choose a reason for hiding this comment

Lakitna Jan 20, 2018

Choose a reason for hiding this comment

ggravlingen commented Jan 20, 2018

coveralls commented Jan 20, 2018 • edited Loading

houndci-bot Jan 20, 2018

Choose a reason for hiding this comment

coveralls commented Jan 20, 2018 • edited Loading

coveralls commented Jan 20, 2018 • edited Loading

coveralls commented Jan 20, 2018 • edited Loading

coveralls commented Jan 20, 2018 • edited Loading

Lakitna Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

ggravlingen Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jan 20, 2018 • edited Loading

ggravlingen commented Jan 20, 2018

Lakitna commented Jan 20, 2018

ggravlingen commented Jan 20, 2018

coveralls commented Jan 19, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

Lakitna Jan 20, 2018 •

edited

Loading

ggravlingen Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading