Transmission Service validation and fixes #155554
Conversation
|
Hey there @engrbm87, @JPHutchins, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the service error handling and validation in the Transmission integration by replacing generic exceptions with translatable service validation errors and standardizing the use of config entry ID constants.
Key changes:
- Replaces
HomeAssistantErrorwithServiceValidationErrorfor better user-facing error messages - Adds translatable exception messages in
strings.json - Standardizes config entry ID handling by using
ATTR_CONFIG_ENTRY_IDinstead of customCONF_ENTRY_ID - Marks service fields as required in
services.yaml
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| homeassistant/components/transmission/strings.json | Adds translatable exception messages for service validation errors |
| homeassistant/components/transmission/services.yaml | Marks all service fields as explicitly required |
| homeassistant/components/transmission/services.py | Refactors error handling to use ServiceValidationError with translations and standardizes constant usage |
| add_torrent: | ||
| fields: | ||
| entry_id: | ||
| required: true |
There was a problem hiding this comment.
Marking this field as required: true is redundant since it's already declared as vol.Required() in the Python service schema at line 33 of services.py. The Python schema validation takes precedence, and adding required: true in the YAML is unnecessary duplication. While not harmful, this redundancy should be removed for consistency with Home Assistant patterns, where required validation is typically handled only in the Python schema.
There was a problem hiding this comment.
These are marked as required as they don't show as mandatory on the UI otherwise and require a checkbox ticking.
| remove_torrent: | ||
| fields: | ||
| entry_id: | ||
| required: true |
There was a problem hiding this comment.
Marking this field as required: true is redundant since it's already declared as vol.Required() in the Python service schema at line 49 of services.py. The Python schema validation takes precedence, and adding required: true in the YAML is unnecessary duplication. While not harmful, this redundancy should be removed for consistency with Home Assistant patterns, where required validation is typically handled only in the Python schema.
| selector: | ||
| text: | ||
| delete_data: | ||
| required: true |
There was a problem hiding this comment.
Marking this field as required: true is redundant since it's already declared with a default value (vol.Optional(ATTR_DELETE_DATA, default=DEFAULT_DELETE_DATA)) in the Python service schema at line 52 of services.py. Fields with default values are optional by nature, so adding required: true creates a contradiction between the YAML and Python definitions. This should be removed.
| required: true |
| start_torrent: | ||
| fields: | ||
| entry_id: | ||
| required: true |
There was a problem hiding this comment.
Marking this field as required: true is redundant since it's already declared as vol.Required() in the Python service schema at line 58 of services.py. The Python schema validation takes precedence, and adding required: true in the YAML is unnecessary duplication. While not harmful, this redundancy should be removed for consistency with Home Assistant patterns, where required validation is typically handled only in the Python schema.
| config_entry: | ||
| integration: transmission | ||
| id: | ||
| required: true |
There was a problem hiding this comment.
Marking this field as required: true is redundant since it's already declared as vol.Required() in the Python service schema at line 58 of services.py. The Python schema validation takes precedence, and adding required: true in the YAML is unnecessary duplication. While not harmful, this redundancy should be removed for consistency with Home Assistant patterns, where required validation is typically handled only in the Python schema.
| required: true |
| stop_torrent: | ||
| fields: | ||
| entry_id: | ||
| required: true |
There was a problem hiding this comment.
Marking this field as required: true is redundant since it's already declared as vol.Required() in the Python service schema at line 62 of services.py. The Python schema validation takes precedence, and adding required: true in the YAML is unnecessary duplication. While not harmful, this redundancy should be removed for consistency with Home Assistant patterns, where required validation is typically handled only in the Python schema.
|
Could I be cheeky and have #155213 reviewed/merged as well, it's slipped down the list and part of getting a quality scale rating. |
2 participants
Proposed change
Raise translatable service validation errors
Pass the service call to _get_coordinator_from_service_data to remove duplicated code
Mark required fields within services.yaml, note the service remove_torrent has delete data set to required within yaml but is still optional (with default) within schema to allow existing automations to continue working.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: