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

fix: check brightness is set before attempting to map the range #1057 #1306

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dhutchison
Copy link

This fixes #1057, or at least my instance of the issue.

It introduces a check to ensure the self._brightness property is set before trying to map it into the range.

I'm not sure what type of testing this project usually required, I've only performed manual tests with a modified version of the component including this one line change and it seems to fix the problem.

@@ -185,7 +185,7 @@ def is_on(self):
@property
def brightness(self):
"""Return the brightness of the light."""
if self.is_color_mode or self.is_white_mode:
if (self.is_color_mode or self.is_white_mode) and self._brightness:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brightness can legitimately be 0, can't it? (ie: this would be false)

Would it be better to check if this is a None or similar value?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 1 might be the lowest legitimate value you can set through the app, but it's a good point & I'll update it. (Been meaning to come back to this all week, but kept forgetting to borrow the offending device out the room before the child went to sleep - dare not wake them for it!)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated after doing a local test

@dhutchison
Copy link
Author

dhutchison commented Apr 23, 2023

Is there anything else that needs done before this can get closer to being merged? Guessing it needs an approval from an admin, (I can't remember how to find who are the writers to the repo on GitHub again...) to allow automated tests to be ran?

@CloCkWeRX do you know? (just stumbled across #781 and a few other issues & seen how active you seem to be in this project)

@CloCkWeRX
Copy link

CloCkWeRX commented Apr 24, 2023

So, it generally helps if you've got someone else experiencing your issue, who can test your patch locally / independently.

In this case it seems pretty safe - so safe I adopted a similar change on my fork (324dcd4):

 +        if self._brightness == None:
 +            return None
 +        if self.is_color_mode or self.is_white_mode:

Maybe also mark this PR as fixes #337

As I run my own fork, I integrate things that seem stable, if you feel like living on the edge - add a custom HACS repo for https://github.com/CloCkWeRX/localtuya-experimental (be sure to review the compare view though! master...CloCkWeRX:localtuya-experimental:master )

@CloCkWeRX
Copy link

@rospogrigio this one or 324dcd4 would be worthwhile merging. I've been running this locally, happily for some time. The code is basically equivalent.

@dhutchison
Copy link
Author

I know how much of a can of worms it could open, but would attempting to add some sort of skeleton for unit tests be a benefit?

@pergolafabio
Copy link

Hi, is this project still active? Seems this PR is going stale?

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

Successfully merging this pull request may close these issues.

"Color" feature depends on "Brightness" feature
3 participants