Add helper management actions#1328
Draft
frenckatron wants to merge 1 commit into
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThree new Spook service groups are added: ChangesNew Spook Service Additions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
frenckatron
force-pushed
the
feat/helper-actions
branch
from
ba59905 to
c3de257
Compare
frenckatron
force-pushed
the
feat/helper-actions
branch
from
c3de257 to
d16b987
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/spook/ectoplasms/input_number/services/decrement.py (1)
21-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix copy-paste error in docstring.
The docstring says "It override the built-in increment service" but this is the decrement service.
📝 Proposed fix
"""Input number entity service, decrease value by a single step. - It override the built-in increment service to allow for a custom amount. + It override the built-in decrement service to allow for a custom amount. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/spook/ectoplasms/input_number/services/decrement.py` around lines 21 - 24, The docstring for the decrement service contains a copy-paste error where it incorrectly references the "increment service" instead of the "decrement service". In the docstring comment that begins with "Input number entity service, decrease value by a single step", change the text "It override the built-in increment service" to "It override the built-in decrement service" to accurately reflect that this is the decrement service, not the increment service.
🧹 Nitpick comments (1)
custom_components/spook/ectoplasms/input_number/services/decrement.py (1)
28-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting common logic into a shared helper function.
Both
increment.pyanddecrement.pyhave nearly identical structure:
- Schema definition for
amountandcycle- Step validation logic (lines 40-46)
- Boundary check pattern (lines 48-56)
The only differences are the operator (+/-), comparison (>/<), and wrap direction (min/max). This duplication could be reduced by extracting a common helper function that accepts the operation and boundary parameters.
♻️ Example refactor approach
Create a common helper in a shared module:
def compute_bounded_value( current: float, delta: float, # positive for increment, negative for decrement minimum: float, maximum: float, cycle: bool, ) -> float: """Compute new value with boundary handling.""" new_value = current + delta if new_value > maximum: return minimum if cycle else maximum if new_value < minimum: return maximum if cycle else minimum return new_valueThen both services could call this helper with their respective delta values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/spook/ectoplasms/input_number/services/decrement.py` around lines 28 - 56, The async_handle_service method in the decrement service contains boundary validation and cycling logic that is nearly identical to the increment service, with only the operation direction and comparison operators differing. Extract the common logic for computing the new bounded value (including cycle wrapping) into a shared helper function in a utilities module or base class that accepts parameters like the current value, delta amount, minimum and maximum boundaries, and the cycle flag. Then update the async_handle_service method in decrement.py to call this helper function instead of implementing the boundary check and cycling logic inline, passing a negative delta value to represent the decrement operation. Apply the same refactoring to the increment.py service to use the same helper function with a positive delta value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/spook/ectoplasms/input_number/services/decrement.py`:
- Around line 28-31: The schema for the amount parameter in the decrement
service lacks validation to ensure it is non-negative, which allows negative
values to bypass the boundary check logic and set values beyond the maximum. Add
validation to the vol.Optional("amount") field in the schema dictionary to
ensure the amount is greater than or equal to zero using an appropriate
validator function that checks this constraint before the decrement operation
uses it.
In `@custom_components/spook/ectoplasms/input_number/services/increment.py`:
- Around line 28-31: The schema definition for the increment service accepts any
float value for the "amount" field without validating that it is non-negative,
which can lead to negative amounts bypassing the maximum boundary check. In the
schema dictionary where vol.Optional("amount") is defined, replace the simple
vol.Coerce(float) with a validator that ensures the amount is greater than or
equal to zero. This will prevent negative values from being accepted and allow
the increment logic to properly enforce both minimum and maximum boundaries.
In `@custom_components/spook/ectoplasms/sensor/services/set_display_precision.py`:
- Around line 40-54: The current implementation updates entity options
immediately within the validation loop, which means if a later entity_id fails
validation, earlier entities will have already been persisted with changes.
Split the operation into two passes: first validate all entity_ids in the
call.data[ATTR_ENTITY_ID] collection and raise HomeAssistantError if any are
invalid (using split_entity_id and entity_registry.async_get checks), then only
after all validations pass, iterate through the entity_ids again to apply the
sensor_options updates with entity_registry.async_update_entity_options for each
valid entity.
In `@tests/ectoplasms/input_number/services/test_increment_decrement.py`:
- Around line 36-103: Add a new parametrized test function after the existing
tests to verify that negative amounts are properly handled or rejected by the
increment.SpookService and decrement.SpookService classes. The test should use
pytest.mark.parametrize to cover at least two scenarios: increment with a
negative amount that would result in a value below the minimum boundary, and
decrement with a negative amount that would result in a value above the maximum
boundary. The test should verify that either an appropriate error is raised or
that the resulting value stays within the entity's minimum and maximum bounds.
---
Outside diff comments:
In `@custom_components/spook/ectoplasms/input_number/services/decrement.py`:
- Around line 21-24: The docstring for the decrement service contains a
copy-paste error where it incorrectly references the "increment service" instead
of the "decrement service". In the docstring comment that begins with "Input
number entity service, decrease value by a single step", change the text "It
override the built-in increment service" to "It override the built-in decrement
service" to accurately reflect that this is the decrement service, not the
increment service.
---
Nitpick comments:
In `@custom_components/spook/ectoplasms/input_number/services/decrement.py`:
- Around line 28-56: The async_handle_service method in the decrement service
contains boundary validation and cycling logic that is nearly identical to the
increment service, with only the operation direction and comparison operators
differing. Extract the common logic for computing the new bounded value
(including cycle wrapping) into a shared helper function in a utilities module
or base class that accepts parameters like the current value, delta amount,
minimum and maximum boundaries, and the cycle flag. Then update the
async_handle_service method in decrement.py to call this helper function instead
of implementing the boundary check and cycling logic inline, passing a negative
delta value to represent the decrement operation. Apply the same refactoring to
the increment.py service to use the same helper function with a positive delta
value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0090d6de-84e3-444b-a413-1d545f876c2d
📒 Files selected for processing (14)
custom_components/spook/ectoplasms/homeassistant/services/expose_entity.pycustom_components/spook/ectoplasms/homeassistant/services/unexpose_entity.pycustom_components/spook/ectoplasms/input_number/services/decrement.pycustom_components/spook/ectoplasms/input_number/services/increment.pycustom_components/spook/ectoplasms/sensor/__init__.pycustom_components/spook/ectoplasms/sensor/services/__init__.pycustom_components/spook/ectoplasms/sensor/services/set_display_precision.pycustom_components/spook/services.yamlcustom_components/spook/translations/en.jsontests/ectoplasms/homeassistant/services/test_voice_assistant_exposure.pytests/ectoplasms/input_number/services/test_increment_decrement.pytests/ectoplasms/sensor/__init__.pytests/ectoplasms/sensor/services/__init__.pytests/ectoplasms/sensor/services/test_set_display_precision.py
Comment on lines
+28
to
+31
| schema = { | ||
| vol.Optional("amount"): vol.Coerce(float), | ||
| vol.Optional("cycle", default=False): bool, | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Add validation that amount is non-negative.
The schema accepts any float for amount without validation. Since the boundary logic at lines 48-56 only checks if new_value < entity._minimum, a negative amount would bypass this check entirely and could set the value above entity._maximum.
Example:
- current_value = 8, minimum = 0, maximum = 10, amount = -5
- new_value = 8 - (-5) = 13
- Since 13 is not < 0, the boundary check is skipped
async_set_value(13)would be called with a value above maximum
🛡️ Proposed fix to validate amount is non-negative
schema = {
- vol.Optional("amount"): vol.Coerce(float),
+ vol.Optional("amount"): vol.All(vol.Coerce(float), vol.Range(min=0)),
vol.Optional("cycle", default=False): bool,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| schema = { | |
| vol.Optional("amount"): vol.Coerce(float), | |
| vol.Optional("cycle", default=False): bool, | |
| } | |
| schema = { | |
| vol.Optional("amount"): vol.All(vol.Coerce(float), vol.Range(min=0)), | |
| vol.Optional("cycle", default=False): bool, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@custom_components/spook/ectoplasms/input_number/services/decrement.py` around
lines 28 - 31, The schema for the amount parameter in the decrement service
lacks validation to ensure it is non-negative, which allows negative values to
bypass the boundary check logic and set values beyond the maximum. Add
validation to the vol.Optional("amount") field in the schema dictionary to
ensure the amount is greater than or equal to zero using an appropriate
validator function that checks this constraint before the decrement operation
uses it.
Comment on lines
+28
to
+31
| schema = { | ||
| vol.Optional("amount"): vol.Coerce(float), | ||
| vol.Optional("cycle", default=False): bool, | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for amount validation or documentation about negative amounts in input_number services
rg -nC3 'amount.*negative|negative.*amount' --type=py
# Check if there's any validation in the base InputNumber entity
ast-grep --lang python --pattern 'class InputNumber' | head -20
# Search for async_set_value implementation to see if it clamps values
rg -nA10 'def async_set_value' --type=py --glob '**/input_number/**'Repository: frenck/spook
Length of output: 150
🏁 Script executed:
# Find and read the increment.py file
find . -type f -name "increment.py" -path "*/input_number/*" | head -5Repository: frenck/spook
Length of output: 222
🏁 Script executed:
# List files in the target directory to understand the structure
fd -type f "increment.py" | head -10Repository: frenck/spook
Length of output: 321
🏁 Script executed:
# Read the increment.py file to see the full context
cat -n ./custom_components/spook/ectoplasms/input_number/services/increment.pyRepository: frenck/spook
Length of output: 2257
🏁 Script executed:
# Also search for async_set_value implementation
rg -n "def async_set_value" --type pyRepository: frenck/spook
Length of output: 150
🏁 Script executed:
# Search for async_set_value in the InputNumber class
rg -n "async_set_value" --type py -A 5 | head -50Repository: frenck/spook
Length of output: 150
🏁 Script executed:
# Look for the InputNumber class definition from homeassistant
rg -n "class InputNumber" --type py | head -10Repository: frenck/spook
Length of output: 150
🏁 Script executed:
# Search for tests related to increment service
find . -type f -name "*test*increment*" -o -name "*increment*test*" | grep -i incrementRepository: frenck/spook
Length of output: 282
🏁 Script executed:
# Search for how the increment service is tested or documented
rg -n "increment" --type py -B 2 -A 2 | head -100Repository: frenck/spook
Length of output: 150
🏁 Script executed:
# Read the increment/decrement test file to see test cases
cat -n ./tests/ectoplasms/input_number/services/test_increment_decrement.pyRepository: frenck/spook
Length of output: 3877
🏁 Script executed:
# Read the decrement.py file to compare logic
cat -n ./custom_components/spook/ectoplasms/input_number/services/decrement.pyRepository: frenck/spook
Length of output: 2257
Add validation that amount is non-negative.
The schema accepts any float for amount without validation. The increment logic only checks if new_value > entity._maximum but does not validate against entity._minimum. A negative amount would bypass the maximum boundary check and allow setting values below the minimum. For example, with current_value=2, minimum=0, maximum=10, and amount=-5: new_value becomes -3, which fails the max check (not > 10) and is passed to async_set_value(-3) without clamping.
🛡️ Proposed fix to validate amount is non-negative
schema = {
- vol.Optional("amount"): vol.Coerce(float),
+ vol.Optional("amount"): vol.All(vol.Coerce(float), vol.Range(min=0)),
vol.Optional("cycle", default=False): bool,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| schema = { | |
| vol.Optional("amount"): vol.Coerce(float), | |
| vol.Optional("cycle", default=False): bool, | |
| } | |
| schema = { | |
| vol.Optional("amount"): vol.All(vol.Coerce(float), vol.Range(min=0)), | |
| vol.Optional("cycle", default=False): bool, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@custom_components/spook/ectoplasms/input_number/services/increment.py` around
lines 28 - 31, The schema definition for the increment service accepts any float
value for the "amount" field without validating that it is non-negative, which
can lead to negative amounts bypassing the maximum boundary check. In the schema
dictionary where vol.Optional("amount") is defined, replace the simple
vol.Coerce(float) with a validator that ensures the amount is greater than or
equal to zero. This will prevent negative values from being accepted and allow
the increment logic to properly enforce both minimum and maximum boundaries.
Comment on lines
+40
to
+54
| for entity_id in call.data[ATTR_ENTITY_ID]: | ||
| if ( | ||
| split_entity_id(entity_id)[0] != DOMAIN | ||
| or entity_registry.async_get(entity_id) is None | ||
| ): | ||
| msg = f"Unknown sensor entity: {entity_id}" | ||
| raise HomeAssistantError(msg) | ||
|
|
||
| sensor_options = dict( | ||
| entity_registry.async_get(entity_id).options.get(DOMAIN, {}) # type: ignore[union-attr] | ||
| ) | ||
| sensor_options[CONF_DISPLAY_PRECISION] = display_precision | ||
| entity_registry.async_update_entity_options( | ||
| entity_id, DOMAIN, sensor_options | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent partial option writes on mixed-validity requests.
If one entity_id is invalid, the service raises after earlier entities may already have been updated, leaving a failed call with partial persisted changes. Validate all targets first, then apply updates in a second pass.
Suggested fix
async def async_handle_service(self, call: ServiceCall) -> None:
"""Handle the service call."""
entity_registry = er.async_get(self.hass)
display_precision = call.data[CONF_DISPLAY_PRECISION]
+ entries: list[er.RegistryEntry] = []
for entity_id in call.data[ATTR_ENTITY_ID]:
- if (
- split_entity_id(entity_id)[0] != DOMAIN
- or entity_registry.async_get(entity_id) is None
- ):
+ entry = entity_registry.async_get(entity_id)
+ if split_entity_id(entity_id)[0] != DOMAIN or entry is None:
msg = f"Unknown sensor entity: {entity_id}"
raise HomeAssistantError(msg)
+ entries.append(entry)
- sensor_options = dict(
- entity_registry.async_get(entity_id).options.get(DOMAIN, {}) # type: ignore[union-attr]
- )
+ for entry in entries:
+ sensor_options = dict(entry.options.get(DOMAIN, {}))
sensor_options[CONF_DISPLAY_PRECISION] = display_precision
entity_registry.async_update_entity_options(
- entity_id, DOMAIN, sensor_options
+ entry.entity_id, DOMAIN, sensor_options
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@custom_components/spook/ectoplasms/sensor/services/set_display_precision.py`
around lines 40 - 54, The current implementation updates entity options
immediately within the validation loop, which means if a later entity_id fails
validation, earlier entities will have already been persisted with changes.
Split the operation into two passes: first validate all entity_ids in the
call.data[ATTR_ENTITY_ID] collection and raise HomeAssistantError if any are
invalid (using split_entity_id and entity_registry.async_get checks), then only
after all validations pass, iterate through the entity_ids again to apply the
sensor_options updates with entity_registry.async_update_entity_options for each
valid entity.
Comment on lines
+36
to
+103
| @pytest.mark.parametrize( | ||
| ("service_cls", "start_value", "amount", "expected"), | ||
| [ | ||
| (increment.SpookService, 9.5, 0.5, 10), | ||
| (increment.SpookService, 9.5, 1, 10), | ||
| (decrement.SpookService, 0.5, 0.5, 0), | ||
| (decrement.SpookService, 0.5, 1, 0), | ||
| ], | ||
| ) | ||
| async def test_input_number_services_clamp_at_range( | ||
| hass: Any, | ||
| service_cls: type[increment.SpookService | decrement.SpookService], | ||
| start_value: float, | ||
| amount: float, | ||
| expected: float, | ||
| ) -> None: | ||
| """Test input number services keep the existing clamping behavior.""" | ||
| entity = MockInputNumberEntity(start_value) | ||
| call = SimpleNamespace(data={"amount": amount, "cycle": False}) | ||
|
|
||
| await service_cls(hass).async_handle_service(entity, call) | ||
|
|
||
| assert entity.value == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("service_cls", "start_value", "amount", "expected"), | ||
| [ | ||
| (increment.SpookService, 9.5, 1, 0), | ||
| (decrement.SpookService, 0.5, 1, 10), | ||
| ], | ||
| ) | ||
| async def test_input_number_services_cycle_at_range( | ||
| hass: Any, | ||
| service_cls: type[increment.SpookService | decrement.SpookService], | ||
| start_value: float, | ||
| amount: float, | ||
| expected: float, | ||
| ) -> None: | ||
| """Test input number services can cycle past their range.""" | ||
| entity = MockInputNumberEntity(start_value) | ||
| call = SimpleNamespace(data={"amount": amount, "cycle": True}) | ||
|
|
||
| await service_cls(hass).async_handle_service(entity, call) | ||
|
|
||
| assert entity.value == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "service_cls", | ||
| [increment.SpookService, decrement.SpookService], | ||
| ) | ||
| async def test_input_number_services_raise_readable_error_for_invalid_amount( | ||
| hass: Any, | ||
| service_cls: type[increment.SpookService | decrement.SpookService], | ||
| ) -> None: | ||
| """Test invalid amounts raise a readable error.""" | ||
| entity = MockInputNumberEntity(1.5) | ||
| call = SimpleNamespace(data={"amount": 0.2, "cycle": False}) | ||
|
|
||
| with pytest.raises( | ||
| ValueError, | ||
| match=escape( | ||
| "Amount 0.2 not valid for input_number.test, " | ||
| "it needs to be a multiple of 0.5" | ||
| ), | ||
| ): | ||
| await service_cls(hass).async_handle_service(entity, call) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add test coverage for negative amounts.
The test suite doesn't cover the case where amount is negative. Based on the critical issues identified in increment.py and decrement.py, negative amounts can bypass boundary checks:
incrementwith negative amount could set values below minimumdecrementwith negative amount could set values above maximum
Add test cases to verify the behavior (or add validation to prevent negative amounts):
🧪 Suggested test cases to add
`@pytest.mark.parametrize`(
("service_cls", "start_value", "amount"),
[
(increment.SpookService, 2, -5), # Would result in -3, below min=0
(decrement.SpookService, 8, -5), # Would result in 13, above max=10
],
)
async def test_input_number_services_with_negative_amount(
hass: Any,
service_cls: type[increment.SpookService | decrement.SpookService],
start_value: float,
amount: float,
) -> None:
"""Test that negative amounts are handled correctly or rejected."""
entity = MockInputNumberEntity(start_value)
call = SimpleNamespace(data={"amount": amount, "cycle": False})
# Either expect proper boundary handling or ValueError
# Behavior depends on fix chosen for the validation issue
await service_cls(hass).async_handle_service(entity, call)
# Assert value stays within bounds
assert entity._minimum <= entity.value <= entity._maximum🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/ectoplasms/input_number/services/test_increment_decrement.py` around
lines 36 - 103, Add a new parametrized test function after the existing tests to
verify that negative amounts are properly handled or rejected by the
increment.SpookService and decrement.SpookService classes. The test should use
pytest.mark.parametrize to cover at least two scenarios: increment with a
negative amount that would result in a value below the minimum boundary, and
decrement with a negative amount that would result in a value above the maximum
boundary. The test should verify that either an appropriate error is raised or
that the resulting value stays within the entity's minimum and maximum bounds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds three helper-focused actions requested from discussions:
sensor.set_display_precision, admin-only, stores the display precision in the sensor entity registry options.homeassistant.expose_entity/homeassistant.unexpose_entity, admin-only, toggles explicit exposure for Assist, Google Assistant, and Alexa.input_number.increment/input_number.decrementnow acceptcycleto wrap around at the configured min/max instead of clamping.Tests
uv run pytest tests/ectoplasms/input_number/services/test_increment_decrement.py tests/ectoplasms/sensor/services/test_set_display_precision.py tests/ectoplasms/homeassistant/services/test_voice_assistant_exposure.py tests/test_ectoplasm_discovery.py -quv run ruff format --check ...uv run ruff check --output-format=github ...uv run pylint ...