-
-
Notifications
You must be signed in to change notification settings - Fork 36.5k
Move url out of zwave strings.json #155312
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
Conversation
Updated the description for the push_weather_data service to include a placeholder for the API URL.
Add description placeholders for API URL in user form.
Removed unnecessary description placeholders from the form display.
Updated the description for the 'push_weather_data' service to include a link to the RainMachine API documentation.
|
Hey there @home-assistant/z-wave, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR moves Z-Wave API documentation URLs out of translatable strings in strings.json and into description placeholders in the config flow. This prevents the URLs from being unnecessarily translated while maintaining the same user-facing functionality.
Key changes:
- Replaced hardcoded URLs in
strings.jsonwith placeholder variables{api_method}and{api_paramaters} - Added URL values to description placeholders in the config flow's
async_step_manualmethod
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| homeassistant/components/zwave_js/strings.json | Replaced hardcoded documentation URLs with placeholder variables in method and parameter descriptions |
| homeassistant/components/zwave_js/config_flow.py | Added URL values for api_method and api_paramaters placeholders in description_placeholders dictionary |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
joostlek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, you're updating the config flow placeholders, but the strings are from the services, this integration requires #154984
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Add API documentation links as description placeholders.
|
I think I added in the correct services... please let me know. |
TheJulianJES
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify: These are URLs used in Home Assistant action services. At the moment, there's no way to use description placeholders in them.
Other integrations mostly used URLs in config flows where it is already possible to use description placeholders to fix the issue with the URLs being in the tranlsations.
Like Joost mentioned, we'll need to wait for #154984 to be merged. Then, we can use a similar implementation to the kitchen_sink integration example used in that PR for Z-Wave.
Added a list of ignored USB devices to prevent them from appearing in serial port selection.
Added API placeholders for documentation links.
Added description placeholders for API method and parameters in the configuration forms.
|
@zweckj any chance I can get some eyes on this. Looks like it failed when I merged the dev branch in... |
|
Please don't ping people, it's considered impolite. You'll need to set up a proper dev environment (e.g. devcontainer) and make sure the pre-commit hooks are passing. It's currently incorrectly formatted (it always was but nobody ran the pipeline I assume) |
| API_PLACEHOLDERS = { | ||
| "api_method": "https://zwave-js.github.io/node-zwave-js/#/api/CCs/index", | ||
| "api_parameters": "https://zwave-js.github.io/node-zwave-js/#/api/CCs/index", | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| API_PLACEHOLDERS = { | |
| "api_method": "https://zwave-js.github.io/node-zwave-js/#/api/CCs/index", | |
| "api_parameters": "https://zwave-js.github.io/node-zwave-js/#/api/CCs/index", | |
| } |
Remove this lines, and only add one placeholder for api_url (URLs are the same), here:
core/homeassistant/components/zwave_js/services.py
Lines 433 to 455 in 9734058
| self._hass.services.async_register( | |
| const.DOMAIN, | |
| const.SERVICE_INVOKE_CC_API, | |
| self.async_invoke_cc_api, | |
| schema=vol.Schema( | |
| vol.All( | |
| { | |
| **TARGET_VALIDATORS, | |
| vol.Required(const.ATTR_COMMAND_CLASS): vol.All( | |
| vol.Coerce(int), vol.Coerce(CommandClass) | |
| ), | |
| vol.Optional(const.ATTR_ENDPOINT): vol.Coerce(int), | |
| vol.Required(const.ATTR_METHOD_NAME): cv.string, | |
| vol.Required(const.ATTR_PARAMETERS): list, | |
| }, | |
| cv.has_at_least_one_key( | |
| ATTR_DEVICE_ID, ATTR_ENTITY_ID, ATTR_AREA_ID | |
| ), | |
| get_nodes_from_service_data, | |
| has_at_least_one_node, | |
| ), | |
| ), | |
| ) |
| API_PLACEHOLDERS = { | ||
| "api_parameters": "https://zwave-js.github.io/node-zwave-js/#/api/CCs/index", | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| API_PLACEHOLDERS = { | |
| "api_parameters": "https://zwave-js.github.io/node-zwave-js/#/api/CCs/index", | |
| } |
Not needed as this is not used in the config flow, only for a service action.
| return self.async_show_form( | ||
| step_id="rf_region", | ||
| data_schema=schema, | ||
| description_placeholders=API_PLACEHOLDERS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| description_placeholders=API_PLACEHOLDERS, |
| step_id="configure_security_keys", | ||
| data_schema=data_schema, | ||
| description_placeholders={ | ||
| "api_method": "https://zwave-js.github.io/node-zwave-js/#/api/CCs/index", | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| step_id="configure_security_keys", | |
| data_schema=data_schema, | |
| description_placeholders={ | |
| "api_method": "https://zwave-js.github.io/node-zwave-js/#/api/CCs/index", | |
| }, | |
| step_id="configure_security_keys", data_schema=data_schema |
| }, | ||
| "method_name": { | ||
| "description": "The name of the API method to call. Refer to the Z-Wave Command Class API documentation (https://zwave-js.github.io/node-zwave-js/#/api/CCs/index) for available methods.", | ||
| "description": "The name of the API method to call. Refer to the Z-Wave Command Class API documentation {api_method} for available methods.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "description": "The name of the API method to call. Refer to the Z-Wave Command Class API documentation {api_method} for available methods.", | |
| "description": "The name of the API method to call. Refer to the Z-Wave Command Class API documentation {api_url} for available methods.", |
| }, | ||
| "parameters": { | ||
| "description": "A list of parameters to pass to the API method. Refer to the Z-Wave Command Class API documentation (https://zwave-js.github.io/node-zwave-js/#/api/CCs/index) for parameters.", | ||
| "description": "A list of parameters to pass to the API method. Refer to the Z-Wave Command Class API documentation {api_parameters} for parameters.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "description": "A list of parameters to pass to the API method. Refer to the Z-Wave Command Class API documentation {api_parameters} for parameters.", | |
| "description": "A list of parameters to pass to the API method. Refer to the Z-Wave Command Class API documentation {api_url} for parameters.", |
Proposed change
Move URL out of strings.json to avoid making it translatable.
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: