Do not request brightness of light by default #982
Conversation
WalkthroughThis pull request introduces a new configuration option Changes
Sequence DiagramsequenceDiagram
participant User
participant LightConverter
participant LightHandler
User->>LightConverter: Configure light service
User-->>LightConverter: Set request_brightness (optional)
LightConverter->>LightHandler: Initialize with requestBrightness
LightHandler-->>LightConverter: Conditionally handle brightness requests
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/converters/light.ts (3)
78-83: Consider consolidating this logic using direct assignment for clarity.
Defining a localrequestBrightnessand then reassigning can be simplified if you want more concise code, though the current approach is also valid and explicit.-let requestBrightness = false; -if (converterConfig.request_brightness === true) { - requestBrightness = true; -} +const requestBrightness = !!converterConfig.request_brightness;🧰 Tools
🪛 GitHub Actions: Verify
[warning] Low code coverage: Only 47.76% statement coverage, 32.75% branch coverage
92-92: Wrap handler creation in a more comprehensive error message.
While you already catch and log an error, adding the current value ofrequestBrightnessto the error log can speed up troubleshooting if misconfiguration leads to unexpected behaviors.-accessory.log.warn(`Failed to setup light for accessory ${accessory.displayName} from expose "${JSON.stringify(expose)}": ${error}`); +accessory.log.warn( + `Failed to setup light for accessory ${accessory.displayName} from expose "${JSON.stringify(expose)}" with requestBrightness=${requestBrightness}: ${error}` +);🧰 Tools
🪛 GitHub Actions: Verify
[warning] Low code coverage: Only 47.76% statement coverage, 32.75% branch coverage
146-146: Explicit naming for therequestBrightnessparameter.
Ensure future maintainers understand its purpose by adding a brief comment (where declared) about controlling brightness queries. This is optional but can enhance clarity.🧰 Tools
🪛 GitHub Actions: Verify
[warning] Low code coverage: Only 47.76% statement coverage, 32.75% branch coverage
docs/light.md (1)
20-20: Hyphenate “converter-specific” for clarity.
When used as a compound modifier (“converter-specific configuration”), it is typically spelled with a hyphen.-Old behavior can be restored using the `request_brightness` option in the converter specific configuration. +Old behavior can be restored using the `request_brightness` option in the converter-specific configuration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Loose punctuation mark.
Context: ...will not be send. -request_brightness: Set totrueto allow the brightness t...(UNLIKELY_OPENING_PUNCTUATION)
CHANGELOG.md (1)
13-15: Apply minor grammar fix and highlight the hyphenation.
Similar to the docs, consider using “converter-specific configuration” for consistent clarity across the project.-Old behavior can be restored using the `request_brightness` option in the converter specific configuration. +Old behavior can be restored using the `request_brightness` option in the converter-specific configuration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: When ‘converter-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... therequest_brightnessoption in the converter specific configuration. (see [#882](https://gi...(SPECIFIC_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)docs/light.md(2 hunks)src/converters/light.ts(5 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~14-~14: When ‘converter-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... the request_brightness option in the converter specific configuration. (see [#882](https://gi...
(SPECIFIC_HYPHEN)
docs/light.md
[uncategorized] ~20-~20: Loose punctuation mark.
Context: ...will not be send. - request_brightness: Set to true to allow the brightness t...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 GitHub Actions: Verify
src/converters/light.ts
[warning] Low code coverage: Only 47.76% statement coverage, 32.75% branch coverage
🔇 Additional comments (2)
src/converters/light.ts (2)
39-39: Add unit tests to verify usage of the new property.
While introducing the optionalrequest_brightness?: boolean;can help prevent unwanted brightness queries, ensure sufficient testing is in place to confirm the behavior is as expected, particularly for edge cases.I can generate a new test file or an updated test suite to cover scenarios where
request_brightnessis toggled. Let me know if you'd like me to open a PR or supply a code snippet!🧰 Tools
🪛 GitHub Actions: Verify
[warning] Low code coverage: Only 47.76% statement coverage, 32.75% branch coverage
191-191: Confirm brightness property remains in sync when togglingrequest_brightness.
This condition excludes brightness fromgetableKeysunlessrequestBrightnessis set. Ensure external calls that rely on brightness updates gracefully handle a missing brightness property.🧰 Tools
🪛 GitHub Actions: Verify
[warning] Low code coverage: Only 47.76% statement coverage, 32.75% branch coverage
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
14-14: Apply a hyphen for ‘converter-specific’
Use a hyphen when ‘converter-specific’ modifies “configuration.”- Old behavior can be restored using the `request_brightness` option in the converter specific configuration. + Old behavior can be restored using the `request_brightness` option in the converter-specific configuration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: When ‘converter-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... therequest_brightnessoption in the converter specific configuration. (see [#882](https://gi...(SPECIFIC_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.vscode/settings.json(1 hunks)CHANGELOG.md(3 hunks)test/light.spec.ts(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~14-~14: When ‘converter-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... the request_brightness option in the converter specific configuration. (see [#882](https://gi...
(SPECIFIC_HYPHEN)
🔇 Additional comments (13)
test/light.spec.ts (9)
184-184: Consistent with removing brightness from default getable keys
The updated line aligns with the new default behavior of not requesting brightness.
510-510: Same approach as line 184
This line is functionally the same as the approach used previously for removing brightness from default getable keys.
560-560: New test block ensures configurability for brightness
This describes a scenario where brightness requests are explicitly allowed. Good to see coverage.
574-574: No changes to review
This appears to be an empty or whitespace-only line.
575-575: Possible mismatch between comment and code
The comment references enabling adaptive lighting, yet the next line setsrequest_brightness. Verify if it's intentional or a leftover reference.
576-576: Explicitly requesting brightness
This configuration is consistent with the test scenario’s need to request brightness. Looks good.
694-694: Same approach as line 184
Again removing brightness from the default getable keys matches the new behavior.
836-836: Only “state” is requested
This device is configured to ignore brightness and color temperature, which aligns with the device’s actual capabilities.
892-892: Only state and color_temp are gettable
This device uses adaptive lighting, but no direct brightness request. Confirmed alignment with new default.CHANGELOG.md (4)
13-13: Aligned with the PR objective
This line describes the new default behavior of not requesting brightness, consistent with the PR's goal.
15-15: Reference to issue #882
No specific revision needed.
117-117: Documentation clarity
Describes added checks to prevent errors—no issues here.
464-464: Changelog improvements
These bullet points are informative clarifications; no changes suggested.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/converters/light.ts (1)
144-144: Consider using parameter property syntaxYou can simplify the code by using TypeScript's parameter property syntax.
- private readonly requestBrightness: boolean, + private readonly requestBrightness: boolean
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(3 hunks)docs/light.md(3 hunks)src/converters/light.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 LanguageTool
docs/light.md
[uncategorized] ~20-~20: Loose punctuation mark.
Context: ...will not be send. - request_brightness: Set to true to allow the brightness t...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (4)
docs/light.md (1)
Line range hint
20-32: Documentation looks good!The new configuration option is well-documented with a clear explanation of its purpose, default value, and rationale. The JSON example effectively demonstrates the usage.
src/converters/light.ts (3)
39-39: LGTM!The interface addition follows TypeScript conventions for optional properties.
78-82: Implementation looks good!The code safely handles the new configuration option with proper type checking and default value initialization.
Line range hint
353-367: Verify adaptive lighting with disabled brightnessThe code requires brightness for adaptive lighting but doesn't consider the
requestBrightnesssetting. This could lead to unexpected behavior when adaptive lighting is enabled but brightness requests are disabled.Consider adding a check in
tryCreateAdaptiveLighting:private tryCreateAdaptiveLighting(service: Service) { // Adaptive lighting is not enabled if (!this.adaptiveLightingEnabled) { return; } // Need at least brightness and color temperature to add Adaptive Lighting - if (this.brightnessExpose === undefined || this.colorTempExpose === undefined) { + if (this.brightnessExpose === undefined || !this.requestBrightness || this.colorTempExpose === undefined) { return; }
| if (this.brightnessExpose !== undefined && exposesCanBeGet(this.brightnessExpose) && this.requestBrightness) { | ||
| keys.push(this.brightnessExpose.property); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider conditional brightness characteristic creation
Currently, the brightness characteristic is always created even when requestBrightness is false, but its value won't be updated. This might lead to stale or incorrect values being displayed.
Consider moving the requestBrightness check to the tryCreateBrightness method to prevent characteristic creation when not needed:
private tryCreateBrightness(features: ExposesEntryWithProperty[], service: Service) {
this.brightnessExpose = features.find(
(e) => e.name === 'brightness' && exposesHasNumericRangeProperty(e) && exposesCanBeSet(e) && exposesIsPublished(e)
) as ExposesEntryWithNumericRangeProperty;
- if (this.brightnessExpose !== undefined) {
+ if (this.brightnessExpose !== undefined && this.requestBrightness) {
this.mainCharacteristics.push(
getOrAddCharacteristic(service, hap.Characteristic.Brightness).on('set', this.handleSetBrightness.bind(this))
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Even if we cannot "get" brightness, it will still be published (we check that when finding the exposes information if I recall correctly). So the characteristic should be created regardless of this new setting.
There was a problem hiding this comment.
@itavero, understood. Thank you for clarifying! The brightness characteristic might still hold value even when it isn’t actively polled, given that it will be published based on the device’s exposes. Let me know if there’s anything else I can help with.
✏️ Learnings added
Learnt from: itavero
PR: itavero/homebridge-z2m#982
File: src/converters/light.ts:189-191
Timestamp: 2025-01-04T19:37:24.409Z
Learning: Even if brightness isn't gettable (when requestBrightness is false), the brightness characteristic remains relevant since it is published by the device. This is reflected in src/converters/light.ts, where we retain the brightness characteristic creation logic.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
🆗 Published SonarCloud and code coverage data. |
|
🚀 This pull request is included in v1.11.0-beta.7. See Release 1.11.0-beta.7 for release notes. |
Should prevent issue mentioned in #882
Summary by CodeRabbit
New Features
request_brightnessconfiguration option for light converters.Documentation
request_brightnessoption.Bug Fixes
illuminance_luxis unavailable.Tests