Migrate nsw_fuel_station to config flow #146001
Conversation
This contains minimal changes required to support config flow and import yaml entries. A PR in progress will restructure and enhance the features further.
There was a problem hiding this comment.
Hi @buxtronix
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @nickw444, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| "fetch_failed": "Failed to fetch fuel price data, check logs", | ||
| "no_matching_stations": "No stations matched the provided search string", | ||
| "missing_fuel_types": "Please select one or more fuel types", | ||
| "station_exists": "Already have entry for that station. Remove and re-add to change fuel types." |
There was a problem hiding this comment.
We should avoid language that uses first-person singular or plural. Here "Already have …" can be read as both "I already have …" and "We already have …" which ends up with bad translations.
Perhaps you can use "An entry for that station already exists. …" here instead.
| "title": "Add new petrol station", | ||
| "description": "Enter a string to search by name or address", | ||
| "data": { | ||
| "search_string": "Name/Address search" |
There was a problem hiding this comment.
This should be sentence-cased -> "Name/address search"
| "select_fuel": { | ||
| "description": "Choose fuel types to display", | ||
| "data": { | ||
| "fuel_types": "Fuel Types" |
There was a problem hiding this comment.
Also needs sentence-casing -> "Fuel types"
|
This PR is rather large, in the sense you describe you only migrate to the config flow, whilst also a coordinator and adjustments to the sensors have been added. General advise would be to keep PR's small, so cut it in pieces:
|
| self.selected_station = next( | ||
| station for station in self.stations if station.code == station_id | ||
| ) | ||
| for existing_entry in self._async_current_entries(): |
There was a problem hiding this comment.
If you create an entry with a unique ID, you don't have to go through a loop. Try using code like this:
await self.async_set_unique_id(YOUR_UNIQUE_STATION_ID)
self._abort_if_unique_id_configured(updates=self._data)| errors: dict[str, str] = {} | ||
|
|
||
| if user_input is not None: | ||
| if ( |
There was a problem hiding this comment.
No need to check here. Make the fuel type required in your schema - it will save this check.
| pytestmark = pytest.mark.usefixtures("mock_setup_entry") | ||
|
|
||
|
|
||
| @pytest.mark.usefixtures("mock_fuelcheckclient") |
There was a problem hiding this comment.
This is already configured in conftest. This can be removed.
| assert result["type"] is FlowResultType.FORM | ||
| assert result["step_id"] == "user" | ||
|
|
||
| result2 = await hass.config_entries.flow.async_configure( |
There was a problem hiding this comment.
Don't create a new variable. Re-use result, to keep the context straight and read-able.
| hass: HomeAssistant, mock_fuelcheckclient: MagicMock | ||
| ) -> None: | ||
| """Test we get the form.""" | ||
| mock_fuelcheckclient.get_reference_data.side_effect = FuelCheckError |
There was a problem hiding this comment.
Are these all errors that can arise? Consider using pytest paramters to create multiple tests at once, within one method.
|
|
||
|
|
||
| @pytest.mark.usefixtures("mock_fuelcheckclient") | ||
| async def test_invalid_fueltype( |
There was a problem hiding this comment.
Check more general on the form exceptions and ensure that it can recover itself after exceptions - so a successful end path.
| @@ -0,0 +1,126 @@ | |||
| """Test the nsw_fuel_station config flow.""" | |||
There was a problem hiding this comment.
Test if a entry is already configured as well.
There was a problem hiding this comment.
Ensure you're getting a 100% config_flow code coverage.
Proposed change
This migrates the nsw_fuel_station integration from legacy YAML config to a config_flow with a much simplified setup experience. Currently, users have to sniff the official web page to fetch station ID, with this change the config flow will allow search for the stations of choice.
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: