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

Make evohome strictly typed #106012

Merged
merged 9 commits into from Dec 21, 2023
Merged

Conversation

zxdavb
Copy link
Contributor

@zxdavb zxdavb commented Dec 18, 2023

Proposed change

With this PR, the evohome integration is now fully typed.

As part of this process, edge-case bugs were found, so some minor changes to the code base were made.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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:

@MartinHjelmare MartinHjelmare changed the title make evohome fully typed Make evohome fully typed Dec 18, 2023
@zxdavb zxdavb changed the title Make evohome fully typed Make evohome strictly typed Dec 18, 2023
homeassistant/components/evohome/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/evohome/water_heater.py Outdated Show resolved Hide resolved
@@ -723,7 +725,7 @@ async def _update_schedule(self) -> None:

assert isinstance(self._evo_device, evo.HotWater | evo.Zone) # mypy check

self._schedule = await self._evo_broker.call_client_api(
self._schedule = await self._evo_broker.call_client_api( # type: ignore[assignment]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to declare self._schedule as optional?

Copy link
Contributor Author

@zxdavb zxdavb Dec 21, 2023

Choose a reason for hiding this comment

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

No, it is never None. It is set to {} during __init__.py, and never is set to None after that.

That is, unlike all the other client APIs wrapped by self._evo_broker.call_client_api(), this method does not ever return None (nor {}):

    async def get_schedule(self) -> _EvoDictT:

IIRC, all the other client APIs only return None:

    async def reset_mode(self) -> None:

Some might say I should use None as a sentinel for it being not yet being retrieved from the internet, but they're both Falsey, so that's OK by me.

In any case, my personal practice is that if something is:

    self._something: dict | None = None

... only during init - usually because an I/O is required - then immediately set to non-None, and never subsequently set to None, then I'll:

    self._something: dict = None  # type: ignore[assignment]

Compare this with self.client_v1, which is optional because at some stage in the code, I might:

   self.client_v1 = None  # type is ev1.EvohomeClient | None

This rule prevents me having to use guard clauses, asserts, and type hints elsewhere in the code.

Copy link
Member

Choose a reason for hiding this comment

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

I saw that we had a check for falsy for self._schedule in another part of the file so I figured it could be an actual case or that it wouldn't matter to type it as optional if we already checked that.

We may be able to improve this by using some of the new typing tools, similar to how we type decorators nowadays. I think we can look at that in a follow up.

Let's keep it like this for now.

Copy link
Member

Choose a reason for hiding this comment

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

Here's an example of what I was thinking:

core/homeassistant/core.py

Lines 604 to 607 in f0104d6

@callback
def async_create_task(
self, target: Coroutine[Any, Any, _R], name: str | None = None
) -> asyncio.Task[_R]:

If the target function is typed, the type checker will understand that the target return value will be the return value of the wrapper too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I think that code is an anachronism:

if not self._schedule.get("DailySchedules"):

... would do.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare MartinHjelmare merged commit aa9f000 into home-assistant:dev Dec 21, 2023
18 checks passed
@zxdavb zxdavb deleted the evo_mypy_work branch December 21, 2023 14:26
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
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.

None yet

2 participants