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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and enable type checks in Rituals Perfume Genie #49947

Merged
merged 3 commits into from May 4, 2021

Conversation

milanmeu
Copy link
Contributor

@milanmeu milanmeu commented May 1, 2021

Proposed change

  • Fix Rituals Perfume Genie typing
  • Remove Rituals Perfume Genie from IGNORED_MODULES
  • Renamed RitualsPerfumeGenieDataUpdateCoordinator -> RitualsDataUpdateCoordinator

I will add more typing and enable strict typing after this PR has merged.

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

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
  • The code has been formatted using Black (black --fast 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@project-bot project-bot bot added this to Needs review in Dev May 1, 2021
@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev May 1, 2021
Copy link
Member

@KapJI KapJI left a comment

Choose a reason for hiding this comment

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

馃憤 for the removing it from ignored modules.

Please see my comments on #49898 regarding storing extra data inside DataUpdateCoordinator. But we can address this in the next PR.

@KapJI
Copy link
Member

KapJI commented May 1, 2021

Actually there's simpler solution. Just add explicit type annotation for coordinator field: #49898 (comment)

coordinator: RitualsDataUpdateCoordinator

@milanmeu
Copy link
Contributor Author

milanmeu commented May 1, 2021

Actually there's simpler solution. Just add explicit type annotation for coordinator field: #49898 (comment)

I think I have done that?

@@ -24,7 +24,10 @@ class DiffuserEntity(CoordinatorEntity):
"""Representation of a diffuser entity."""

def __init__(
self, diffuser: Diffuser, coordinator: CoordinatorEntity, entity_suffix: str
self,
Copy link
Member

Choose a reason for hiding this comment

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

I can't see it. Should be here.

class DiffuserEntity(CoordinatorEntity):
  coordinator: RitualsDataUpdateCoordinator

Copy link
Contributor Author

@milanmeu milanmeu May 1, 2021

Choose a reason for hiding this comment

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

I thought adding an annotation to the function parameter was enough.
I have added the field annotation in 5efca6b.
Why is this only necessary for the custom coordinator and why does mypy not warn about this?

Copy link
Member

@KapJI KapJI May 2, 2021

Choose a reason for hiding this comment

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

Why is this only necessary for the custom coordinator

This way mypy can know self.coordinator in entity has type RitualsDataUpdateCoordinator and not DataUpdateCoordinator.

why does mypy not warn about this?

Because you didn't use coordinator._device outside _async_update_data. But if you add more fields there and start using them in entity, mypy won't able to find them. So it's just an extra annotation to let mypy know that self.coordinator is custom entity.

Copy link
Member

@KapJI KapJI left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you for fixing this! You need to rebase to fix conflict in mypy.ini.

@MartinHjelmare please take a look.

@epenet
Copy link
Contributor

epenet commented May 4, 2021

Needs rebase again following merge of #50030

Dev automation moved this from By Code Owner to Review in progress May 4, 2021
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @milanmeu 馃憤

Dev automation moved this from Review in progress to Reviewer approved May 4, 2021
@frenck frenck merged commit a0feee0 into home-assistant:dev May 4, 2021
Dev automation moved this from Reviewer approved to Done May 4, 2021
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants