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.

Sign up for GitHub

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

Jump to bottom

[various] Lamp handlers expose min/max Color Temperature in state description #17641

Merged
merged 8 commits into from
Nov 1, 2024
Merged

[various] Lamp handlers expose min/max Color Temperature in state description #17641

merged 8 commits into from
Nov 1, 2024

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Oct 27, 2024 •
edited
Loading

Lamp handlers expose min/max Color Temperature in state description

Resolves #17638

Signed-off-by: AndrewFG [email protected]

@andrewfg andrewfg added enhancement An enhancement or new feature for an existing add-on additional testing preferred The change works for the pull request author. A test from someone else is preferred though. awaiting other PR Depends on another PR labels Oct 27, 2024
@andrewfg andrewfg self-assigned this Oct 27, 2024
@andrewfg
Fix typo
9f2a38a 
Signed-off-by: AndrewFG <[email protected]>
@andrewfg
Performance improvements
4738738 
Signed-off-by: AndrewFG <[email protected]>
stefan-hoehn
stefan-hoehn approved these changes Oct 27, 2024
View reviewed changes
Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

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

govee lgtm

@lolodomo
Copy link
Contributor

As discussed in the hue PR, there is a better option for the provider.
Let's wait for the merge of PR #17637 first.

@andrewfg andrewfg changed the title Lamp handlers expose min/max Colour Temperature in state description [various] Lamp handlers expose min/max Colour Temperature in state description Oct 28, 2024
@andrewfg andrewfg mentioned this pull request Oct 28, 2024
Closed
12 tasks
lolodomo
lolodomo approved these changes Oct 28, 2024
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@andrewfg andrewfg removed the awaiting other PR Depends on another PR label Oct 28, 2024
@lolodomo
Copy link
Contributor

@andrewfg : what kind of tests exactly are you waiting for?

@andrewfg
Copy link
Contributor Author

what kind of tests exactly are you waiting for?

Ideally I would like 1) to wait for your CT widget and then 2) ask users to test the min/max correct range.
However in absence of both 1) and 2) perhaps I should write some JUnit tests to test on a mocked thing / channel?

wborn
wborn reviewed Oct 29, 2024
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

US English would be preferred.

...fx/src/main/java/org/openhab/binding/lifx/internal/handler/LifxStateDescriptionProvider.java Outdated Show resolved Hide resolved
...ain/java/org/openhab/binding/nanoleaf/internal/handler/NanoLeafStateDescriptionProvider.java Outdated Show resolved Hide resolved
@andrewfg
use US english spelling
2f8b5dc 
Signed-off-by: AndrewFG <[email protected]>
@andrewfg andrewfg changed the title [various] Lamp handlers expose min/max Colour Temperature in state description [various] Lamp handlers expose min/max Color Temperature in state description Oct 29, 2024
@andrewfg
Copy link
Contributor Author

perhaps I should write some JUnit tests to test on a mocked thing / channel?

@lolodomo I spent some hours trying to write proper Junit tests with mocks etc. but in the end it would require modifying the bindings too substantially and risks to break more than it fixes. So instead, I have done some local actual and thought experiments on the code. And as a result I made some minor improvements. And I am now convinced that it is ready to merge.

@andrewfg andrewfg removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Oct 31, 2024
lolodomo
lolodomo approved these changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo lolodomo merged commit 2080c85 into openhab:main Nov 1, 2024
5 checks passed
@lolodomo lolodomo added this to the 4.3 milestone Nov 1, 2024
@andrewfg andrewfg deleted the color-temp-set-min-max branch November 6, 2024 15:45
KaaNee pushed a commit to KaaNee/openhab-addons that referenced this pull request Nov 8, 2024
@andrewfg @KaaNee
[various] Lamp handlers expose min/max Color Temperature in state des…
ce8e8c7 
…cription (openhab#17641)

* Lamp handlers expose min/max Colour Temperature in state description
* add color temperature validit and range checks

Signed-off-by: AndrewFG <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wborn wborn wborn left review comments

@lolodomo lolodomo lolodomo approved these changes

@stefan-hoehn stefan-hoehn stefan-hoehn approved these changes

@cweitkamp cweitkamp Awaiting requested review from cweitkamp cweitkamp is a code owner

Assignees

@andrewfg andrewfg

Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Add min/max color temperature capabilities to channel state descriptions
4 participants
@andrewfg @lolodomo @stefan-hoehn @wborn