Skip to content

Record Overkiz quality scale #133719

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
Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from
Draft

Record Overkiz quality scale #133719

wants to merge 8 commits into from

Conversation

iMicknl
Copy link
Contributor

@iMicknl iMicknl commented Dec 21, 2024 •
edited
Loading

Proposed change

Bronze

Silver

  • config-entry-unloading - Support config entry unloading

    core/homeassistant/components/overkiz/__init__.py

    Line 168 in bbb5f9e

    async def async_unload_entry(
  • log-when-unavailable - If internet/device/service is unavailable, log once when unavailable and once when back connected

    core/homeassistant/components/overkiz/coordinator.py

    Lines 75 to 84 in bbb5f9e

    except TooManyConcurrentRequestsException as exception:
    raise UpdateFailed("Too many concurrent requests.") from exception
    except TooManyRequestsException as exception:
    raise UpdateFailed("Too many requests, try again later.") from exception
    except MaintenanceException as exception:
    raise UpdateFailed("Server is down for maintenance.") from exception
    except InvalidEventListenerIdException as exception:
    raise UpdateFailed(exception) from exception
    except (TimeoutError, ClientConnectorError) as exception:
    raise UpdateFailed("Failed to connect.") from exception
  • entity-unavailable - Mark entity unavailable if appropriate

    core/homeassistant/components/overkiz/coordinator.py

    Lines 75 to 84 in bbb5f9e

    except TooManyConcurrentRequestsException as exception:
    raise UpdateFailed("Too many concurrent requests.") from exception
    except TooManyRequestsException as exception:
    raise UpdateFailed("Too many requests, try again later.") from exception
    except MaintenanceException as exception:
    raise UpdateFailed("Server is down for maintenance.") from exception
    except InvalidEventListenerIdException as exception:
    raise UpdateFailed(exception) from exception
    except (TimeoutError, ClientConnectorError) as exception:
    raise UpdateFailed("Failed to connect.") from exception

    core/homeassistant/components/overkiz/entity.py

    Line 38 in bbb5f9e

    self._attr_available = self.device.available
  • action-exceptions - Service actions raise exceptions when encountering failures
  • reauthentication-flow - Reauthentication flow

    core/homeassistant/components/overkiz/config_flow.py

    Line 318 in bbb5f9e

    async def async_step_reauth(
  • parallel-updates - Set Parallel updates
  • test-coverage - Above 95% test coverage for all integration modules
  • integration-owner - Has an integration owner
  • docs-installation-parameters - The documentation describes all integration installation parameters
    https://www.home-assistant.io/integrations/overkiz#configuration
  • docs-configuration-parameters - The documentation describes all integration configuration options

Gold

  • entity-translations - Entities have translated names
  • entity-device-class - Entities use device classes where possible
  • devices - The integration creates devices

    core/homeassistant/components/overkiz/entity.py

    Line 90 in bbb5f9e

    return DeviceInfo(
  • entity-category - Entities are assigned an appropriate EntityCategory
  • entity-disabled-by-default - Integration disables less popular (or noisy) entities

    core/homeassistant/components/overkiz/sensor.py

    Line 233 in bbb5f9e

    entity_registry_enabled_default=False,
  • discovery - Can be discovered

    core/homeassistant/components/overkiz/config_flow.py

    Line 286 in bbb5f9e

    async def async_step_zeroconf(
  • stale-devices - Clean up stale devices
  • diagnostics - Implements diagnostics

    core/homeassistant/components/overkiz/diagnostics.py

    Line 17 in bbb5f9e

    async def async_get_config_entry_diagnostics(
  • exception-translations - Exception messages are translatable
  • icon-translations - Icon translations
  • reconfiguration-flow - Integrations should have a reconfigure flow
  • dynamic-devices - Devices added after integration setup
  • discovery-update-info - Integration uses discovery info to update network information

    core/homeassistant/components/overkiz/config_flow.py

    Lines 290 to 292 in bbb5f9e

    properties = discovery_info.properties
    gateway_id = properties["gateway_pin"]
    hostname = discovery_info.hostname
  • repair-issues - Repair issues and repair flows are used when user intervention is needed
  • docs-use-cases - The documentation describes use cases to illustrate how this integration can be used
  • docs-supported-devices - The documentation describes known supported / unsupported devices
  • docs-supported-functions - The documentation describes the supported functionality, including entities, and platforms
  • docs-data-update - The documentation describes how data is updated
  • docs-known-limitations - The documentation describes known limitations of the integration (not to be confused with bugs)
  • docs-troubleshooting - The documentation provides troubleshooting information
  • docs-examples - The documentation provides automation examples the user can use.

Platinum

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.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @vlebourl, @tetienne, @nyroDev, @Tronix117, @alexfp14, mind taking a look at this pull request as it has been labeled with an integration (overkiz) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of overkiz can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign overkiz Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@iMicknl iMicknl force-pushed the overkiz/quality_scale branch from 0ac1fd5 to ad803ee Compare December 22, 2024 10:35
@iMicknl iMicknl force-pushed the overkiz/quality_scale branch from ad803ee to 5f89d9e Compare December 23, 2024 09:44
@iMicknl iMicknl marked this pull request as ready for review December 23, 2024 16:06
@iMicknl iMicknl requested a review from a team as a code owner December 23, 2024 16:06
joostlek
joostlek requested changes Jan 16, 2025
View reviewed changes
homeassistant/components/overkiz/quality_scale.yaml
@@ -0,0 +1,68 @@
rules:
Copy link
Member

Choose a reason for hiding this comment

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

Why is the update_interval of the coordinator a parameter if you only create 1, and it is a static value? I think you could even optimize this to have the coordinator figure out what update_interval it should poll at (this way you don't need that extra set_update_interval function to be publicly accessible) (it would also not surprise me if i discover later that this is used in more ways)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move the initial logic to the coordinator, and change set_update_interval to a private setter? (even though still the same in Python). Would that be better? Personally I don't have any preference.

The reason we do set it in __init__ is because we do know if the user has the local API configured, which we don't know at the moment in the coordinator.

core/homeassistant/components/overkiz/__init__.py

Line 127 in 9b2faf2

if api_type == APIType.LOCAL:

homeassistant/components/overkiz/quality_scale.yaml
@@ -0,0 +1,68 @@
rules:
Copy link
Member

Choose a reason for hiding this comment

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

alarm_control_panel.py -> all the *args of the entity description are just a string, so I am not sure why they can be typed as a list.

Also, I am wondering if it would be possible to instead use a Callable[[<the type of the executor>], None] as parameter and just put in a lambda instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For alarm_control_panel all devices only use a single argument (and a string). Overkiz command execution supports multiple comands args, and thus you can pass a list as well.

homeassistant/components/overkiz/quality_scale.yaml
docs-removal-instructions: done
test-before-setup: done
docs-high-level-description: done
config-flow-test-coverage: done
Copy link
Member

Choose a reason for hiding this comment

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

test_form_only_cloud_supported -> What is this testing? We don't assert the last result. Also, stale docstring

homeassistant/components/overkiz/quality_scale.yaml
docs-removal-instructions: done
test-before-setup: done
docs-high-level-description: done
config-flow-test-coverage: done
Copy link
Member

Choose a reason for hiding this comment

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

test_form_local_happy_flow -> Let's also test the end result + stale docstring

homeassistant/components/overkiz/quality_scale.yaml
docs-removal-instructions: done
test-before-setup: done
docs-high-level-description: done
config-flow-test-coverage: done
Copy link
Member

Choose a reason for hiding this comment

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

test_form_invalid_auth_cloud + others -> Let's have every test end in either ABORT or CREATE_ENTRY to also test the flow is able to recover from an error

homeassistant/components/overkiz/quality_scale.yaml
docs-removal-instructions: done
test-before-setup: done
docs-high-level-description: done
config-flow-test-coverage: done
Copy link
Member

Choose a reason for hiding this comment

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

test_dhcp_flow -> Also test unique id

Copy link
Member

Choose a reason for hiding this comment

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

same for the zeroconf

homeassistant/components/overkiz/quality_scale.yaml
docs-installation-parameters: done
integration-owner: done
parallel-updates: todo
test-coverage: todo
Copy link
Member

Choose a reason for hiding this comment

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

in test_init.py, let's replace assert await async_setup_component(hass, DOMAIN, {}) with the hass.config_entries.async_setup() one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#143398

@home-assistant home-assistant bot marked this pull request as draft January 16, 2025 16:55
@home-assistant
Copy link

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.

@github-actions GitHub Actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Mar 17, 2025
@github-actions github-actions bot closed this Mar 24, 2025
@iMicknl iMicknl reopened this Mar 24, 2025
@iMicknl iMicknl removed the stale label Mar 24, 2025
@iMicknl iMicknl marked this pull request as ready for review April 17, 2025 21:08
@home-assistant home-assistant bot requested a review from joostlek April 17, 2025 21:08
@iMicknl iMicknl marked this pull request as draft April 18, 2025 11:45
@iMicknl iMicknl removed the request for review from joostlek April 18, 2025 11:46
This was referenced Apr 18, 2025
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
@github-actions GitHub Actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Jun 21, 2025
@github-actions github-actions bot closed this Jun 28, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jun 29, 2025
@iMicknl iMicknl reopened this Jun 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joostlek joostlek joostlek requested changes

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
by-code-owner cla-signed code-quality integration: overkiz Quality Scale: No score quality-scale stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iMicknl @joostlek