Skip to content
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

Add coordinator to evohome and prune async_update code #119432

Merged
merged 42 commits into from
Jul 23, 2024

Conversation

zxdavb
Copy link
Contributor

@zxdavb zxdavb commented Jun 11, 2024

Proposed change

This PR makes changes that will prepare this integration for config flow:

  • it adds a very simple DataUpdateCoordinator
  • it splits out authentication (which uses username, password) from other options (i.e. location_idx) to better match the config flow pattern
  • it thins out async_update() code that was out-of-place

I have some tests for this code, but will add them in a subsequent PR.

It does not include config flow.

Later PRs will make further changes (e.g. adding tests), until the final PR, which will add config flow. The objective is for a sequence of small PRs, rather than one large PR.

There are no breaking changes and no new functionality.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • it moves some code from async_setup() to EvoSession, so that that code is now accessible to (say) config flow
  • it moves other code from async_setup() to EvoBroker, so that that code is now accessible to (say) options flow
  • some symbols (e.g. private attrs) have been renamed

This PR is a pre-requisite for #119008, which has become otherwise untenable

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@zxdavb zxdavb marked this pull request as draft June 11, 2024 20:47
@zxdavb zxdavb changed the title Evo coordinator2 Add co-ordinator to evohome and export async_update code into broker Jun 11, 2024
@zxdavb zxdavb marked this pull request as ready for review June 11, 2024 22:19
@zxdavb zxdavb changed the title Add co-ordinator to evohome and export async_update code into broker Add co-ordinator to evohome and move async_update code into broker Jun 11, 2024
@MartinHjelmare MartinHjelmare changed the title Add co-ordinator to evohome and move async_update code into broker Add coordinator to evohome and move async_update code into broker Jun 12, 2024
Copy link
Contributor

@elupus elupus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong way to go. Put a helper in init instead if you need to share this between config flow and legacy setup.

Putting this in the broker moves setup logic into class that could just have easily been injected. Its generally easier to handle cleanup if you inject things and avoid construction of sub objects in init

@zxdavb zxdavb marked this pull request as draft June 18, 2024 21:35
@elupus
Copy link
Contributor

elupus commented Jun 21, 2024

Should have been enough

@zxdavb
Copy link
Contributor Author

zxdavb commented Jun 21, 2024

I don't need to rerun script/setup?

(I'm not at home to check)

@epenet
Copy link
Contributor

epenet commented Jun 21, 2024

CI should be fixed with #120080

Copy link
Member

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some observations that should be handled

homeassistant/components/evohome/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/evohome/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/evohome/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/evohome/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/evohome/coordinator.py Outdated Show resolved Hide resolved
@home-assistant
Copy link

home-assistant bot commented Jul 1, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft July 1, 2024 16:23
@zxdavb zxdavb marked this pull request as ready for review July 2, 2024 21:30
@home-assistant home-assistant bot requested a review from Kane610 July 2, 2024 21:30
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think there is room for improvement, but at this point, it's acceptable, so let's merge this just to improve it a bit and continue iterating from there

@zxdavb
Copy link
Contributor Author

zxdavb commented Jul 22, 2024

Once again it's a design thing, you're in control of the library but you get into situations where you have to work around your own design.

The background is that my library is a 'faithful' async port (requests to aiohttp) of a much older library not written by me.

By faithful, I mean I chose to keep the original namespace, to help others switch over, and that is the root cause of many of these challenges (e.g. # noqa: SLF001).

Only recently have I accepted the wisdom of deviating from the original design, and that will allow me to address these 'situations'.

Presently, I am stuck down a rabbit hole making changes to evohome-async, including adding an Auth class as per: https://developers.home-assistant.io/docs/api_lib_auth/

In the meantime, I would love to get this PR merged.

@zxdavb zxdavb changed the title Add coordinator to evohome and prune async_update code into EvoSession Add coordinator to evohome and prune async_update code Jul 23, 2024
@joostlek joostlek merged commit 42b9c04 into home-assistant:dev Jul 23, 2024
22 checks passed
@zxdavb zxdavb deleted the evo_coordinator2 branch July 23, 2024 15:59
@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants