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

Add backlight inversion option (Fixes #5) #18

Closed
wants to merge 3 commits into from

Conversation

sajattack
Copy link

I saw this issue going through @kattni's library status issue and it looked like a quick fix so I jumped in. The new API lets you add an inverted field to the backlight object used in the constructor, and also lets you specify in the set_backlight method. In both cases it defaults to True, the old behaviour, so this should be a non-breaking API change.

@kattni kattni requested a review from a team November 12, 2018 22:19
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I wouldn't add the attribute to the backlight object because its type is unknown. Instead, I'd have it separate state that is added to self via the constructor-only. It doesn't need to be in set_backlight because it won't change after it's hooked up.

@brennen
Copy link
Contributor

brennen commented Nov 15, 2018

I think @kattni may have a slightly different fix for this issue baked into some work she's doing on this library, which is getting a pretty substantial overhaul for unrelated reasons. Probably should wait for her to comment on this one.

@kattni
Copy link
Contributor

kattni commented Nov 15, 2018

@sajattack I am currently doing a complete rework of this library. It will include the fix for the backlight issue. I really appreciate you putting in this PR, but I'm going to be taking care of the fix myself as part of the rework. Thank you again for your work!

@kattni kattni closed this Nov 15, 2018
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.

None yet

4 participants