Migrate meteoalarm integration to config flow #124290
Conversation
home-assistant
bot
added
cla-signed
config-flow
|
Hey there @rolfberkenbosch, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
…home-assistant into 2024/8/meteoalarm_config_flow
|
Still working on local tests |
| try: | ||
| api = Meteoalert(country, province, language) | ||
| api = Meteoalert( | ||
| entry.data[CONF_COUNTRY], | ||
| entry.data[CONF_PROVINCE], | ||
| entry.data[CONF_LANGUAGE], | ||
| ) | ||
| except KeyError: | ||
| _LOGGER.error("Wrong country digits or province name") | ||
| LOGGER.error("Wrong country digits or province name") | ||
| return |
There was a problem hiding this comment.
This should be moved to __init__.py. We should store that in entry.runtime_data (please make sure you extend the typing of ConfigEntry for type checking
There was a problem hiding this comment.
@joostlek Thanks for looking at this so quick.
This is my first config-flow conversion.
I've been having some trouble finding documentation for types such as ConfigEntry, ConfigFlowResult, etc. Is there anything available that you know of, besides using the source?
There was a problem hiding this comment.
The source and all the other integrations out there. Reworking the docs is something on my list
There was a problem hiding this comment.
This should be moved to
__init__.py. We should store that inentry.runtime_data(please make sure you extend the typing of ConfigEntry for type checking
I am struggling to understand what you mean here. Do you have an example?
There was a problem hiding this comment.
AirGradient has an example, type MeteoAlarmConfigEntry = ConfigEntry[...]
|
Oh lol, it was ready for review so I already gave it a shot :) |
|
Remaining: Test config import |
| LOGGER.error("Wrong country digits or province name") | ||
| errors["base"] = "wrong_country_or_province" |
There was a problem hiding this comment.
Country can't be wrong now right?
Is there a predefined list of provinces per country? Maybe it's worth introducing an extra step to select the province so people can't mess it up (as in, if we can avoid people looking up what to fill in and make mistakes, that's always nice)
There was a problem hiding this comment.
I had a look some time ago into this integration and it simply isn't possible to fetch the list dynamically at runtime.
As far as I'm aware, there's also no KeyError being throw in the library to know that the province provided is incorrect. When the wrong province is given, it will simply return no alerts (since it will never match your wrong province.)
The list of countries is provided on their https://feeds.meteoalarm.org/ page only, so hardcoding is the only solution here (no html parsing allowed). The list of provinces is also available in a Google docs json file available through their redistribution hub. That's a 30MB file with shapes included. I think hardcoding is also the only solution here, but note that some countries do have a lot of provinces.
There was a problem hiding this comment.
This is a good one to check with others. I think it's nice to make it foolproof, because you also explain it won't work otherwise.
But that dict should ideally live in the library
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Unstale pls |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
/unstale |
|
Apologies for the lack of updates in this issue. I have been putting my time in developing a custom integration, while gaining experience on how to do this properly. Going forward with this integration now. 👍 |
Proposed change
Add config flow to
meteoalarmintegrationType 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: