Add Data Grand Lyon integration#167946
Add Data Grand Lyon integration#167946Crocmagnon wants to merge 5 commits intohome-assistant:devfrom
Conversation
11f3728 to
af56b68
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new tcl Home Assistant integration for Lyon (TCL) public transit and Vélo'v bike-sharing, including config flows with subentries and sensor entities backed by a coordinator.
Changes:
- Adds TCL integration implementation (config flow + coordinator + sensor platform + translations/manifest/quality scale).
- Adds automated tests for the integration’s config flow and subentry flows.
- Registers the integration in generated indices and adds the upstream dependency to requirements.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/components/tcl/test_config_flow.py | Adds config flow + subentry flow tests for TCL integration. |
| tests/components/tcl/conftest.py | Adds shared fixture to patch async_setup_entry during tests. |
| tests/components/tcl/init.py | Adds tests package marker for TCL. |
| requirements_test_all.txt | Adds transports-commun-lyon to the test requirements set. |
| requirements_all.txt | Adds transports-commun-lyon to the runtime requirements set. |
| homeassistant/generated/integrations.json | Registers the new tcl integration for discovery/metadata. |
| homeassistant/generated/config_flows.py | Registers tcl as supporting config flows. |
| homeassistant/components/tcl/strings.json | Adds UI strings for config flow, subentries, and entity translations. |
| homeassistant/components/tcl/sensor.py | Implements stop and Vélo'v sensors using the coordinator pattern. |
| homeassistant/components/tcl/quality_scale.yaml | Declares current quality-scale rule status for the new integration. |
| homeassistant/components/tcl/manifest.json | Adds manifest metadata and pins dependency version. |
| homeassistant/components/tcl/coordinator.py | Adds coordinator to fetch/merge passages and fetch station status. |
| homeassistant/components/tcl/const.py | Defines constants for keys/subentry types and passage count. |
| homeassistant/components/tcl/config_flow.py | Implements main config flow, reauth, and subentry flows. |
| homeassistant/components/tcl/init.py | Wires up coordinator, update listener, and forwards platform setup. |
| CODEOWNERS | Assigns ownership for new integration code and tests. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
homeassistant/components/tcl/manifest.json:1
- The manifest includes several empty fields (
dependencies,homekit,ssdp,zeroconf). If these are not intentionally required by hassfest for this integration, consider removing them to reduce noise and keep the manifest focused on meaningful declarations.
{
1e1d56d to
ea8ae45
Compare
ea8ae45 to
2125708
Compare
2125708 to
c056273
Compare
| velov[subentry.subentry_id] = velov_result | ||
|
|
||
| if (stop_subentries or velov_subentries) and not stops and not velov: | ||
| raise UpdateFailed("Error fetching DataGrandLyon data: all requests failed") |
There was a problem hiding this comment.
The UpdateFailed message is user/diagnostic-facing and currently uses an internal-looking name (DataGrandLyon) and doesn’t provide actionable context. Consider using the proper integration name (Data Grand Lyon) and adding minimal detail (e.g., number of stop/station requests and that all failed) to improve troubleshooting without being overly verbose.
| raise UpdateFailed("Error fetching DataGrandLyon data: all requests failed") | |
| raise UpdateFailed( | |
| f"Error fetching Data Grand Lyon data: all {len(stop_subentries)} " | |
| f"stop request(s) and {len(velov_subentries)} Vélo'v station " | |
| "request(s) failed" | |
| ) |
| LOGGER.warning( | ||
| "Error fetching passages for stop %s: %s", | ||
| subentry.subentry_id, | ||
| result, | ||
| ) |
There was a problem hiding this comment.
These warnings drop the traceback even though you have the actual exception object. Logging with exc_info=... (or using LOGGER.exception where appropriate) would preserve stack traces, which materially improves field diagnostics when API/library failures occur.
| if isinstance(velov_result, BaseException): | ||
| LOGGER.warning( | ||
| "Error fetching Vélo'v station %s: %s", | ||
| subentry.subentry_id, | ||
| velov_result, | ||
| ) |
There was a problem hiding this comment.
These warnings drop the traceback even though you have the actual exception object. Logging with exc_info=... (or using LOGGER.exception where appropriate) would preserve stack traces, which materially improves field diagnostics when API/library failures occur.
New integration for Data Grand Lyon open data platform. Supports config subentries for transit stops (with estimated/theoretical passage merging) and bike sharing stations (with electrical/mechanical bike breakdown). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c056273 to
ab0f34c
Compare
| @classmethod | ||
| @callback | ||
| def async_get_supported_subentry_types( | ||
| cls, config_entry: ConfigEntry | ||
| ) -> dict[str, type[ConfigSubentryFlow]]: | ||
| """Return subentry types supported by this integration.""" | ||
| return { | ||
| SUBENTRY_TYPE_STOP: StopSubentryFlowHandler, | ||
| SUBENTRY_TYPE_VELOV: VelovSubentryFlowHandler, | ||
| } |
There was a problem hiding this comment.
Can we start with a single sub entry type?
| async def async_step_reconfigure( | ||
| self, user_input: dict[str, Any] | None = None | ||
| ) -> ConfigFlowResult: | ||
| """Handle reconfiguration of the main config entry.""" | ||
| errors: dict[str, str] = {} | ||
|
|
||
| if user_input is not None: | ||
| data: dict[str, Any] = {} | ||
| if username := user_input.get(CONF_USERNAME): | ||
| data[CONF_USERNAME] = username | ||
| if password := user_input.get(CONF_PASSWORD): | ||
| data[CONF_PASSWORD] = password | ||
|
|
||
| if error := await self._test_connection(data): | ||
| errors["base"] = error | ||
| else: | ||
| return self.async_update_reload_and_abort( | ||
| self._get_reconfigure_entry(), | ||
| data=data, | ||
| ) | ||
|
|
||
| return self.async_show_form( | ||
| step_id="reconfigure", | ||
| data_schema=self.add_suggested_values_to_schema( | ||
| STEP_USER_DATA_SCHEMA, | ||
| self._get_reconfigure_entry().data, | ||
| ), | ||
| errors=errors, | ||
| ) | ||
|
|
||
| async def async_step_reauth( | ||
| self, entry_data: Mapping[str, Any] | ||
| ) -> ConfigFlowResult: | ||
| """Handle initiation of re-authentication.""" |
There was a problem hiding this comment.
Can we keep reconfigure and reauth out for the first PR?
| stops: dict[str, list[TclPassage]] = field(default_factory=dict) | ||
| velov: dict[str, VelovStation | None] = field(default_factory=dict) |
There was a problem hiding this comment.
no need for a default factory right?
There was a problem hiding this comment.
Why do you run a single coordinator? You could split it in theory
There was a problem hiding this comment.
I'm not very familiar with HA core, this is my first experience. How do you suggest to split the coordinator? One for each sub-entry type?
| discovery-update-info: | ||
| status: exempt | ||
| comment: Devices can't be discovered. | ||
| discovery: | ||
| status: exempt | ||
| comment: Devices can't be discovered. |
There was a problem hiding this comment.
rather explain that this is a service
| for subentry_id, subentry in entry.subentries.items(): | ||
| if subentry.subentry_type == SUBENTRY_TYPE_STOP: | ||
| async_add_entities( | ||
| ( | ||
| DataGrandLyonStopSensor(coordinator, subentry, description) | ||
| for description in STOP_SENSOR_DESCRIPTIONS | ||
| ), | ||
| config_subentry_id=subentry_id, | ||
| ) | ||
| elif subentry.subentry_type == SUBENTRY_TYPE_VELOV: | ||
| async_add_entities( | ||
| ( | ||
| DataGrandLyonVelovSensor(coordinator, subentry, description) | ||
| for description in VELOV_SENSOR_DESCRIPTIONS | ||
| ), | ||
| config_subentry_id=subentry_id, | ||
| ) |
There was a problem hiding this comment.
ideally we run async_add_entities as little as possible, so if we can limit it to 2 by iterating over the list (which you can make smaller with the entry.get_subentries_of_type helper)
There was a problem hiding this comment.
I'm not sure I understand. The config_subentry_id param for async_add_entities forces me to call it for each sub-entry, doesn't it?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
- Remove Vélo'v (bike-sharing) subentry type; keep only TCL transit stop - Remove reconfigure and reauth flows (defer to follow-up PR) - Simplify coordinator to stop-only data, remove unused default_factory - Use entry.get_subentries_of_type() in sensor setup - Downgrade quality scale to bronze (silver requires reauthentication-flow) - Update quality_scale.yaml: mark reauthentication-flow and reconfiguration-flow as todo, clarify discovery exemption comments Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thank you for your review @joostlek, I've pushed a commit to address your feedback. I got a bit carried away in my initial commit so the PR was indeed not as minimal as it could've been. |
| STEP_USER_DATA_SCHEMA = vol.Schema( | ||
| { | ||
| vol.Required(CONF_USERNAME): str, | ||
| vol.Required(CONF_PASSWORD): str, | ||
| } |
There was a problem hiding this comment.
Allow credentials to be optional here if the integration supports an unauthenticated setup (tests submit an empty dict); otherwise the flow can never be completed without entering both fields.
| if error := await self._test_connection(data): | ||
| errors["base"] = error | ||
| else: | ||
| return self.async_create_entry(title="Data Grand Lyon", data=data) |
There was a problem hiding this comment.
Avoid running the connection test when both username and password are omitted (currently an empty submission will still reach _test_connection and be treated as an auth error).
| if error := await self._test_connection(data): | |
| errors["base"] = error | |
| else: | |
| return self.async_create_entry(title="Data Grand Lyon", data=data) | |
| if data: | |
| if error := await self._test_connection(data): | |
| errors["base"] = error | |
| else: | |
| return self.async_create_entry(title="Data Grand Lyon", data=data) |
| Returns None on success, or an error key for the errors dict. | ||
| """ | ||
| if not data.get(CONF_USERNAME): | ||
| return "invalid_auth" |
There was a problem hiding this comment.
Don’t return invalid_auth just because username is missing; if anonymous mode is supported, treat missing credentials as a successful no-op test (or skip this method entirely) so the flow can create the entry.
| return "invalid_auth" | |
| return None |
| stops: dict[str, list[TclPassage]] = {} | ||
| for i, subentry in enumerate(stop_subentries): | ||
| result = stop_results[i] | ||
| if isinstance(result, BaseException): |
There was a problem hiding this comment.
Handle asyncio.CancelledError separately (or only treat Exception as a per-stop failure) so cancellation isn’t swallowed/logged as a normal fetch error.
| if isinstance(result, BaseException): | |
| if isinstance(result, asyncio.CancelledError): | |
| raise result | |
| if isinstance(result, Exception): |
| "domain": "data_grandlyon", | ||
| "name": "Data Grand Lyon", | ||
| "codeowners": ["@Crocmagnon"], | ||
| "config_flow": true, | ||
| "documentation": "https://www.home-assistant.io/integrations/data_grandlyon", |
There was a problem hiding this comment.
Align the PR description with the code in this PR: the current implementation only adds transit stop subentries/sensors (no bike sharing subentries or passage merging logic are present).
Username and password are required since the only subentry type (TCL stop) needs authentication. Update the schema to vol.Required and drop the no-credentials test; fix test_form_already_configured to pass credentials and mock the API call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ee5bd7a to
3c1bafe
Compare
| stop_results: list[list[TclPassage] | BaseException] = await asyncio.gather( | ||
| *stop_tasks, return_exceptions=True | ||
| ) | ||
|
|
||
| stops: dict[str, list[TclPassage]] = {} | ||
| for i, subentry in enumerate(stop_subentries): | ||
| result = stop_results[i] | ||
| if isinstance(result, BaseException): | ||
| LOGGER.warning( | ||
| "Error fetching passages for stop %s: %s", | ||
| subentry.subentry_id, | ||
| result, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Avoid swallowing asyncio.CancelledError by treating only Exception results from asyncio.gather(..., return_exceptions=True) as failures (or explicitly re-raising cancellations).
| if err.status in (401, 403): | ||
| return "invalid_auth" | ||
| return "cannot_connect" | ||
| except ClientError, TimeoutError: |
There was a problem hiding this comment.
Fix the exception handler syntax by using a tuple in the except clause so the module parses on current Python versions.
| except ClientError, TimeoutError: | |
| except (ClientError, TimeoutError): |
| data: dict[str, Any] = {} | ||
| if username := user_input.get(CONF_USERNAME): | ||
| data[CONF_USERNAME] = username | ||
| if password := user_input.get(CONF_PASSWORD): | ||
| data[CONF_PASSWORD] = password |
There was a problem hiding this comment.
Store required credentials directly from user_input instead of conditionally adding them, since the current truthy checks can drop empty strings and create entries missing required keys.
| data: dict[str, Any] = {} | |
| if username := user_input.get(CONF_USERNAME): | |
| data[CONF_USERNAME] = username | |
| if password := user_input.get(CONF_PASSWORD): | |
| data[CONF_PASSWORD] = password | |
| data: dict[str, Any] = { | |
| CONF_USERNAME: user_input[CONF_USERNAME], | |
| CONF_PASSWORD: user_input[CONF_PASSWORD], | |
| } |
| ) -> SubentryFlowResult: | ||
| """Handle the user step to add a new stop.""" | ||
| entry = self._get_entry() | ||
| if not entry.data.get(CONF_USERNAME): |
There was a problem hiding this comment.
Require both username and password (or otherwise verify auth is configured) before allowing the stop subentry flow to proceed, since checking only CONF_USERNAME can allow creating subentries with incomplete credentials.
| if not entry.data.get(CONF_USERNAME): | |
| if not entry.data.get(CONF_USERNAME) or not entry.data.get(CONF_PASSWORD): |
| { | ||
| "domain": "data_grandlyon", | ||
| "name": "Data Grand Lyon", | ||
| "codeowners": ["@Crocmagnon"], | ||
| "config_flow": true, | ||
| "documentation": "https://www.home-assistant.io/integrations/data_grandlyon", | ||
| "integration_type": "service", | ||
| "iot_class": "cloud_polling", | ||
| "quality_scale": "bronze", | ||
| "requirements": ["data-grand-lyon-ha==0.5.0"] |
There was a problem hiding this comment.
Align the PR description with the implementation, since this integration currently only adds transit stop subentries/sensors and does not include the described bike-station support.
Proposed change
New integration for Data Grand Lyon open data platform. Supports config subentries for transit stops (with estimated/theoretical passage merging) and bike sharing stations (with electrical/mechanical bike breakdown).
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:
Enregistrement d'écran_20260411_104705.webm
Enregistrement d'écran_20260411_104606.webm