-
-
Notifications
You must be signed in to change notification settings - Fork 36k
Set station name as device name in GIOS #155762
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
base: dev
Are you sure you want to change the base?
Conversation
|
Hey there @bieniu, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
c05e0af to
1156200
Compare
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 removes the user-configurable device name from the GIOS integration config flow and instead uses the station name from the GIOS API. For backward compatibility, existing configurations with a custom name will continue to use it, while new setups will use the station name with translation support.
Key changes:
- Config flow no longer allows users to set custom device names
- Device names are now derived from the station name via API (with translation support for new configs)
- Backward compatibility maintained for existing configs with
CONF_NAMEin their data
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/gios/config_flow.py | Removed CONF_NAME import and name field from config flow schema |
| homeassistant/components/gios/sensor.py | Updated device info creation to use station name with translation support, while maintaining backward compatibility for existing configs with custom names |
| homeassistant/components/gios/strings.json | Added device translation key for station name formatting |
| tests/components/gios/conftest.py | Updated mock config entry to remove CONF_NAME from test data |
| tests/components/gios/test_config_flow.py | Removed CONF_NAME import and from test config data |
| tests/components/gios/test_init.py | Added test for device naming behavior based on config data (with/without CONF_NAME) |
| tests/components/gios/test_sensor.py | Updated entity IDs in tests to reflect new naming based on station name |
| tests/components/gios/snapshots/*.ambr | Updated snapshots to reflect new entity naming scheme |
Comments suppressed due to low confidence (1)
homeassistant/components/gios/sensor.py:18
- The
CONF_NAMEimport is still present but is now only used with.get()method on line 187. Since this is for backward compatibility with existing config entries, the import is actually still needed and should remain. However, it would be clearer to add a comment explaining this is kept for backward compatibility with existing configurations.
from homeassistant.const import CONCENTRATION_MICROGRAMS_PER_CUBIC_METER, CONF_NAME
| }, | ||
| "device": { | ||
| "station": { | ||
| "name": "Station {station_name}" |
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.
TBH I see no reason to add the word "Station" to the device name. Other integrations simply use the name/location of the measurement/weather station.
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.
I removed the prefix.
| else: | ||
| device_info_kwargs["translation_key"] = "station" | ||
| device_info_kwargs["translation_placeholders"] = {"station_name": station_name} | ||
| self._attr_device_info = DeviceInfo(**device_info_kwargs) |
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.
I would move the creation of device info to the coordinator module:
self.device_info = DeviceInfo(
entry_type=DeviceEntryType.SERVICE,
identifiers={(DOMAIN, str(gios.station_id))},
manufacturer=MANUFACTURER,
name=config_entry.data.get(CONF_NAME) or gios.station_name,
configuration_url=URL.format(station_id=gios.station_id),
)and only add here self._attr_device_info = coordinator.device_info
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.
I moved device info to coordinator.
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.
Tests are failing
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| mode=SelectSelectorMode.DROPDOWN, | ||
| ), | ||
| ), | ||
| vol.Optional(CONF_NAME, default=self.hass.config.location_name): str, |
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.
Name was removed here, but not removed from the translations file (string.json), which still needs to be cleaned up.
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.
Good catch. Removed.
| ) | ||
|
|
||
| station_id = gios.station_id | ||
| assert station_id is not None |
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.
We shouldn't be using assert in production code. In general the use means we either have a logic error (or could improve logic) or there might be a typing issue/improvement that could result the need for it.
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.
We have this check here because the API client can be initialized with an optional station_id parameter, but in our case, the client is always initialized with a specific station ID. The client without a station ID is used in config_flow to retrieve the list of available stations.
https://github.com/bieniu/gios/blob/871ea652a27216f87645a0161b823f500cd7a944/gios/__init__.py#L51
Here is Gios client initialization code:
| gios = await Gios.create(websession, station_id) |
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.
I added if TYPE_CHECKING to avoid this code in production code.
Here is PR: home-assistant/home-assistant.io#41588 |
| dict({ | ||
| 'config_entry': dict({ | ||
| 'data': dict({ | ||
| 'name': 'Home', |
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.
name should remain.
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.
Updated.
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.
Please align the config entry for tests; sometimes the name is there, sometimes it's not. And I think we should leave the station name as Home to prove that the "old" tests pass with the new code.
One more thing... please, if you think the PR is ready, click Ready for review button. Without that, it's unclear whether to look at the changes or wait because you're still working.
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.
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.
Thanks for pointing that out. I appreciate the reminder. I was planning to take one more pass over everything before marking the PR as ready for review.
Also, thanks for being so consistent and thorough with the reviews.


Breaking change
Proposed change
This change supersedes: #155741
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: