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.

Sign up for GitHub

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

Jump to bottom

Address late review of SwitchBee #78412

Merged
merged 8 commits into from Sep 16, 2022
Merged

Address late review of SwitchBee #78412

merged 8 commits into from Sep 16, 2022

Conversation

jafar-atili
Copy link
Contributor

@jafar-atili jafar-atili commented Sep 13, 2022 •
edited

Breaking change

Proposed change

#70201

  • Removed "Light as switches" option from config_flow.py and removed its logic from the code [@frenck ]
  • Removed the Options support of including/excluding devices by their type + removed its logic from the code [@frenck]
  • test_config_flow data moved to fixtures directory and used fixtures helpers to load it [@MartinHjelmare]
  • DataUpdateCoordinator code moved from __init__.py to coordinator.py [@frenck]
  • Fixed other CR comments raised by @frenck, @MartinHjelmare and @epenet
  • Fixed issue where offline devices were still available in the UI
  • Improved comments that explains unclear code
  • Bumped pyswitchbee to 1.4.8 jafar-atili/pySwitchbee@20dd614

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

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.
  • 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.

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 Sep 13, 2022
@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Sep 13, 2022
epenet
epenet reviewed Sep 14, 2022
View reviewed changes
Copy link
Contributor

@epenet epenet left a comment

Choose a reason for hiding this comment

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

Please also take into account all my comments from #78386 and apply them to the switch platform

.coveragerc Outdated Show resolved Hide resolved
homeassistant/components/switchbee/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/coordinator.py Show resolved Hide resolved
homeassistant/components/switchbee/switch.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/switch.py Show resolved Hide resolved
@jafar-atili
Copy link
Contributor Author

@epenet, Thanks for your comments! I'll start working on them right away.
I'd appreciate if you can help me understand @frenck #70201 (comment) about the UpdateCoordinator.

@epenet epenet mentioned this pull request Sep 14, 2022
Merged
23 tasks
@jafar-atili jafar-atili mentioned this pull request Sep 14, 2022
Merged
22 tasks
@jafar-atili jafar-atili requested review from epenet and thecode and removed request for epenet September 14, 2022 14:20
@jafar-atili jafar-atili marked this pull request as ready for review September 14, 2022 14:23
@jafar-atili jafar-atili marked this pull request as draft September 14, 2022 14:55
@jafar-atili jafar-atili marked this pull request as ready for review September 14, 2022 15:34
@jafar-atili jafar-atili requested review from epenet and removed request for thecode September 14, 2022 17:23
@jafar-atili jafar-atili mentioned this pull request Sep 14, 2022
Merged
8 tasks
epenet
epenet approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@epenet epenet left a comment

Choose a reason for hiding this comment

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

For my part it looks OK, but I suggest that you wait for confirmation from @frenck and @MartinHjelmare since it includes changes they requested.

Dev automation moved this from By Code Owner to Reviewer approved Sep 16, 2022
MartinHjelmare
MartinHjelmare requested changes Sep 16, 2022
View reviewed changes
homeassistant/components/switchbee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/switch.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/switch.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/switch.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/switch.py Outdated Show resolved Hide resolved
homeassistant/components/switchbee/switch.py Outdated Show resolved Hide resolved
tests/components/switchbee/test_config_flow.py Outdated Show resolved Hide resolved
Dev automation moved this from Reviewer approved to Review in progress Sep 16, 2022
@MartinHjelmare
Copy link
Member

Please add a link to a changelog or GitHub commit compare view for the version bump in the PR description. Thanks!

@jafar-atili
Copy link
Contributor Author

Please add a link to a changelog or GitHub commit compare view for the version bump in the PR description. Thanks!

Done (jafar-atili/pySwitchbee@20dd614)

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Sep 16, 2022 •
edited

Please tag a GitHub release that correspond to the PyPI release when publishing a new release. This will make it easier for the community to track the development of the library.

I recommend automating this with a GitHub action.

Dev automation moved this from Review in progress to Reviewer approved Sep 16, 2022
@MartinHjelmare MartinHjelmare merged commit 491177e into home-assistant:dev Sep 16, 2022
Dev automation moved this from Reviewer approved to Done Sep 16, 2022
@epenet
Copy link
Contributor

epenet commented Sep 16, 2022

Now that it is merged, can I suggest that you add switchbee to strict typing as a follow-up PR?
I think the component should be quite close already...

frenck
frenck reviewed Sep 16, 2022
View reviewed changes
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.

One small (late) comment

homeassistant/components/switchbee/switch.py
@property
def available(self) -> bool:
"""Return True if entity is available."""
return self._is_online and self.coordinator.last_update_success
Copy link
Member

Choose a reason for hiding this comment

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

The last update check is provided by the parent entity.

return self._is_online and super().available

@jafar-atili
Copy link
Contributor Author

Now that it is merged, can I suggest that you add switchbee to strict typing as a follow-up PR?

I think the component should be quite close already...

Yeah Sure :)

@jafar-atili jafar-atili deleted the frenck_feedback branch September 16, 2022 13:46
@jafar-atili jafar-atili mentioned this pull request Sep 16, 2022
Merged
23 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@frenck frenck frenck left review comments

@thecode thecode thecode left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

@epenet epenet epenet approved these changes

Assignees
No one assigned
Labels
by-code-owner cla-signed has-tests integration: switchbee
Projects
Dev
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jafar-atili @MartinHjelmare @epenet @frenck @thecode @homeassistant