Use config subentries in nederlandse spoorwegen #139857
Conversation
|
Hey there @YarmoM, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
acbf756 to
c8ef47a
Compare
|
The sub entries currently have an annoying issue. After adding one it is nicely added but does not start to show data until after a reboot. Edit: after making it possible to reload the integration (fe06495) that seems to work as well. Edit edit: solving this turned out to be quite simple. I have added an update listener that reloads the integration after changes. This works like a charm. Commit: fbc4009. |
|
Pylint fails due to "Return type should be FlowResult in async_step_user". I think this is unavoidable since this PR makes use of subconfig entries. I'm not entirely sure why Mypy fails, it gives an error about a function not returning a value but that is incorrect. It might be caused by a lack of type hinting in the API being used. |
|
@Martreides issue is fixed, if you rebase the CI should pass in your PR, too #143502 |
e0795e4 to
77a1968
Compare
| @dataclass | ||
| class NederlandseSpoorwegenData: | ||
| """Data structure for runtime data.""" | ||
|
|
||
| nsapi: ns_api.NSAPI |
| nsapi: ns_api.NSAPI | ||
|
|
||
|
|
||
| async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: |
There was a problem hiding this comment.
extend ConfigEntry with the type of entry.runtime_data and use that throughout the code
| nsapi = ns_api.NSAPI(entry.data[CONF_API_KEY]) | ||
| loop = asyncio.get_running_loop() | ||
| await loop.run_in_executor(None, nsapi.get_stations) | ||
| entry.runtime_data = NederlandseSpoorwegenData(nsapi) |
There was a problem hiding this comment.
only keep things in the try block that can raise
| try: | ||
| nsapi = ns_api.NSAPI(entry.data[CONF_API_KEY]) | ||
| loop = asyncio.get_running_loop() | ||
| await loop.run_in_executor(None, nsapi.get_stations) |
There was a problem hiding this comment.
please use hass.async_add_executor_job instead
| raise ConfigEntryAuthFailed( | ||
| "Could not instantiate the Nederlandse Spoorwegen API." | ||
| ) from ex | ||
| else: |
There was a problem hiding this comment.
we can leave the YAML for a followup
| ROUTE_SCHEMA = vol.Schema( | ||
| { | ||
| vol.Required(CONF_NAME): cv.string, | ||
| vol.Required(CONF_FROM): cv.string, | ||
| vol.Required(CONF_TO): cv.string, | ||
| vol.Optional(CONF_VIA): cv.string, | ||
| vol.Optional(CONF_TIME): cv.time, | ||
| } | ||
| from .const import ( | ||
| CONF_STATION_FROM, | ||
| CONF_STATION_TO, | ||
| CONF_STATION_VIA, | ||
| CONF_TIME, | ||
| MIN_TIME_BETWEEN_UPDATES, | ||
| ) | ||
|
|
||
| ROUTES_SCHEMA = vol.All(cv.ensure_list, [ROUTE_SCHEMA]) | ||
|
|
||
| PLATFORM_SCHEMA = SENSOR_PLATFORM_SCHEMA.extend( | ||
| {vol.Required(CONF_API_KEY): cv.string, vol.Optional(CONF_ROUTES): ROUTES_SCHEMA} | ||
| ) | ||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def setup_platform( | ||
| async def async_setup_entry( | ||
| hass: HomeAssistant, | ||
| config: ConfigType, | ||
| add_entities: AddEntitiesCallback, | ||
| discovery_info: DiscoveryInfoType | None = None, | ||
| entry: ConfigEntry, | ||
| async_add_entities: AddConfigEntryEntitiesCallback, | ||
| ) -> None: | ||
| """Set up the departure sensor.""" | ||
|
|
||
| nsapi = ns_api.NSAPI(config[CONF_API_KEY]) | ||
|
|
||
| try: | ||
| stations = nsapi.get_stations() | ||
| except ( | ||
| requests.exceptions.ConnectionError, | ||
| requests.exceptions.HTTPError, | ||
| ) as error: | ||
| _LOGGER.error("Could not connect to the internet: %s", error) | ||
| raise PlatformNotReady from error | ||
| except RequestParametersError as error: | ||
| _LOGGER.error("Could not fetch stations, please check configuration: %s", error) | ||
| return | ||
|
|
||
| sensors = [] | ||
| for departure in config.get(CONF_ROUTES, {}): |
There was a problem hiding this comment.
So we still want to import the existing YAML config so the user only has to remove their YAML remnants and they're ready
| @Throttle(MIN_TIME_BETWEEN_UPDATES) | ||
| def update(self) -> None: | ||
| @Throttle(timedelta(seconds=MIN_TIME_BETWEEN_UPDATES)) | ||
| async def async_update(self) -> None: |
There was a problem hiding this comment.
is it possible to keep this one untouched?
| """Initialize the sensor.""" | ||
| self._hass = hass |
There was a problem hiding this comment.
| self._hass = hass |
self.hass is set once the entity is added to hass
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Silence mypy error caused by imported package.
b2b6078 to
5be0477
Compare
|
Thanks for the review @joostlek. It will take some time to get through all of the comments, I'll start working on it. |
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
| @@ -0,0 +1,60 @@ | |||
| rules: | |||
There was a problem hiding this comment.
Please make a separate PR with the quality scale addition.
Breaking change
Proposed change
Major overhaul of the nederlandse_spoorwegen integration. The integration is currently a legacy one which can only be configured through config.yaml. This PR is intended as a first big step towards a Bronze quality for this integration. It moves the integration from yaml into a config flow. This alone required major changes to the integration including changing function to use asyncio.
Note that the change also implements Config Subentries, that functionality is so fresh that this is currently the only actual integration making use of it.
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: