-
Notifications
You must be signed in to change notification settings - Fork 12
Rework config flow, add Reauth Flow, add Options Flow (polling interval), add zip release github workflow, set min version to 2024.12 #42
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
Conversation
Added "(Needs Translation)" to strings that are new
…ate entry, add update listener
|
Wow! Great work!! |
|
@gdgib
|
|
Excellent!! I'll go through this a bit to make sure I can follow it all in some detail, but I expect this to be the backbone of 1.4.0 to be released within a week. Any sense of timeline on the other PR? The issues you noticed overlapped with ones I noticed, too, but as this isn't the only integration I'm working on right now it sounds like you'll beat me to the punch. |
|
I have all of my other integrations in a stable state (hopefully no breaking API changes in the near future), so I can probably shoot for within your week timeframe. Don't let that hold you off from dealing with this PR. Edit: @gdgib |
|
I'll review this in a few hours. Let me know if you want me to release 1.4.0 with or without the other PR. I'm not picky, and you have a few days to decide I figure. And I'll get discussions turned on when I'm back at a computer myself. |
gdgib
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.
Only one question, otherwise everything looks great.
|
Oh, and @RobertD502 discussions are on. |
|
@gdgib Any chance of getting this pulled in? I'm hoping to finish up the next big PR tonight - having these changes merged in the repo will help make the next PR go smoothly. Let me know if there are any other issues you wanted to discuss regarding this PR! |
|
Had some personal events this weekend that required my time. Thanks for explaining the change in the release process. I'd love to think about how to generalize some of that work so that we can add the same features to other integrations I maintain. |
|
FWIW, @RobertD502 you have an invitation to the team now. |
|
No problem at all. Completely understand personal time taking priority! I have essentially reworked every aspect of the integration for the next PR (still have to rework the current I'll try to find some time to squeeze that in between a home reno and replacing fried components on my washer, dryer, mini split inverter, and ceiling fan control boards (Stupid snow/ice storm). |
|
I never thought I'd meet someone whose project list intimidated ME, but here you are. I'm try to keep an eye on issues then, and leave the coding to you entirely for a while just to avoid creating merge conflicts later. If I don't hear from you in the next 2 weeks, I'll reach out to see how things are going. In the meantime I'm testing these changes, and you have an invitation to the org so you'll get write access to the repo. I'm trying to level up a few integrations right now, so my focus will be split. While I want to be involved more here, if you aren't hearing from me enough, use your judgement and write access as you see fit. |
Alright this is a big one, so I'll break down the changes.
Translations
en.jsonfile, but missing from the other translation files. Afterwards, the key counts were validated between theen.jsonfile and every other translation.(Needs Translation)appended to make it easier for contributors toctrl + Ftheir way through while translating.config_flow.py
version 2and pretty much completely reworked.Configurebutton. For new config flows, a default prefilled suggestion of 60 seconds is used.single instancecheck has been removed as I don't see why users shouldn't be able to add multiple accounts - the integration creates a new VeSync instance during a config flow/within the update coordinator, so, there is no session sharing going on that would cause problems (like when reusing an aiohttp session from HA). However, a check still remains to prevent users from trying to add multiple configs with the same account.Reauth flowhas been added, which is triggered when the pyvesyncloginmethod fails - I wish the backend was more granular and raised an exception for a specific code/error that is encountered when wrong credentials are used, but we have to work with what we got.Options Flowhas been added to allow users that have the integration set up to change the polling interval via the Configure button in the UI from the VeSync integration page.Example of new Config Flow
Example of Options Flow
vesyn_options_flow.mov
init.py
ConfigEntryAuthFailedexception gets raised, which allows the user to enter the reauth flow.async_config_entry_first_refresh()method to fetch initial data.loaded_platformslist is now also stored in Home Assistant data as we need to use it when unloading a config entry (more on that below).async_unload_entryhas been reworked to use the storedloaded_platformslist. Previously, thePLATFORMSconstant's dict keys were used to create a list, but that causes issues. If a user doesn't have at least 1 entity of each type of platform defined in thePLATFORMSdict, an error is logged by Home Assistant during a config unload - aside from removing a config entry, a config unload is also done when an option has been changed in the options flow (polling interval) as part of the config reload. For example, I only have a humidifier that has nofanorbuttonentities. When using thePLATFORMsdict keys, an error is raised about unloading the fan and button platforms since they were never loaded. By using the stored loaded_platforms list, HA will only try to unload platforms that are actually being used by a vesync config entry.async_migrate_entryhas been added. This will migrate the config entry for any user that set the integration up previously (who have a config entry version of 1) to the new config entry version 2, which adds a default polling_interval of 60 seconds to those users' config entries. Of course, with the new options flow added, they can change the polling interval to something else via the UI.README
MYbutton for users to be able to open the repo directly in HACS without going through adding a custom repo, but, of course, adding the repo manually is still a valid option.Github Workflows
linterworkflow has all actions bumped to the latest version & the python-setup action now uses a.python-versionfile to specify the python version (3.13at the moment to reflect the python version HA uses).release.ymlhas been added, which will create a zip ofcustom_components/vesyncasvesync.zipand, whenever you publish a release, add it as an asset to the release. By doing so, you can have a metric of how many times a release has been downloaded + the number of times downloaded stat will also show in HACS.hacs.json
render_readmekey has been removed as it is no longer used (HACS now requires integrations to have a README file).homeassistantkey has been added to specify a minimum HA version required (2024.12in this case). HA introduced some changes to the options flow in 2024.12 and the newly added options flow follows those changes. By setting the min version of HA we can prevent users, with a lower version of HA, from trying to install a release of the integration that includes the option flow code. I could have the option flow use the older code and we not set the minimum HA version, but anyone on 2024.12 or greater will get deprecation warnings in their logs. It's a good idea to keep track of what HA code changes, that this integration ends up implementing, aren't backwards compatible and bump the version as it is needed.Related issues
I know this is a lot, but I hope the rundown helped provide some insight into the code changes.