-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Change Z-Wave JS API model to match zwave-js #20793
Conversation
When the backend is merged, you can change the label from "wait for backend" to "backend merged" and we'll follow to merge then. Also release the draft from the PR when you feel like this is ready. |
Thanks I thought we had already merged the backend, my mistake. I will try to test this over the weekend so that we can move forward with both |
WalkthroughThe recent changes streamline property naming and structure within the Z-Wave JS integration code. Specifically, they involve renaming properties for consistency and merging additional properties directly into the main interface. These updates improve code readability and maintainability by standardizing property names and simplifying interface definitions. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
tested backend and frontend, everything looks good 👍🏾 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range comments (4)
src/panels/config/integrations/integration-panels/zwave_js/zwave_js-provisioned.ts (2)
Line range hint
122-122
: Avoid using non-null assertions. Consider adding appropriate checks or handling potential null values.- this._provisioningEntries = await fetchZwaveProvisioningEntries(this.hass!, this.configEntryId); + if (!this.hass || !this.configEntryId) { + return; + } + this._provisioningEntries = await fetchZwaveProvisioningEntries(this.hass, this.configEntryId);
Line range hint
4-5
: Optimize imports by removing unused imports or converting them to type-only imports if they are only used for type annotations.- import { mdiCheckCircle, mdiCloseCircleOutline, mdiDelete } from "@mdi/js"; - import { html, LitElement } from "lit"; - import { customElement, property, state } from "lit/decorators"; - import memoizeOne from "memoize-one"; - import { DataTableColumnContainer } from "../../../../../components/data-table/ha-data-table"; - import { - ZwaveJSProvisioningEntry, - fetchZwaveProvisioningEntries, - SecurityClass, - unprovisionZwaveSmartStartNode, - } from "../../../../../data/zwave_js"; - import { showConfirmationDialog } from "../../../../../dialogs/generic/show-dialog-box"; - import "../../../../../layouts/hass-tabs-subpage-data-table"; - import { HomeAssistant, Route } from "../../../../../types"; - import { configTabs } from "./zwave_js-config-router"; + import type { mdiCheckCircle, mdiCloseCircleOutline, mdiDelete } from "@mdi/js"; + import type { html, LitElement } from "lit"; + import type { customElement, property, state } from "lit/decorators"; + import type memoizeOne from "memoize-one"; + import type { DataTableColumnContainer } from "../../../../../components/data-table/ha-data-table"; + import type { + ZwaveJSProvisioningEntry, + fetchZwaveProvisioningEntries, + SecurityClass, + unprovisionZwaveSmartStartNode, + } from "../../../../../data/zwave_js"; + import type { showConfirmationDialog } from "../../../../../dialogs/generic/show-dialog-box"; + import type "../../../../../layouts/hass-tabs-subpage-data-table"; + import type { HomeAssistant, Route } from "../../../../../types"; + import type { configTabs } from "./zwave_js-config-router";Also applies to: 5-11, 13-14
src/data/zwave_js.ts (2)
Line range hint
4-4
: Review the enum declarations to ensure they are correctly defined and used throughout the codebase.Consider simplifying the enum declarations and ensuring they are not declared as
const
unless necessary.Also applies to: 17-17, 73-73, 88-88, 100-107, 127-127, 380-380
Line range hint
240-240
: Specify types more precisely instead of usingany
to improve type safety and code maintainability.- [prop: string]: any; + [prop: string]: string | number | boolean;Also applies to: 393-393, 439-439
@@ -82,7 +82,7 @@ class ZWaveJSProvisioned extends LitElement { | |||
filterable: true, | |||
sortable: true, | |||
template: (entry) => { | |||
const securityClasses = entry.security_classes; | |||
const securityClasses = entry.securityClasses; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure that the security class names are correctly localized.
The search results indicate that the localization keys for security classes are used in the codebase, but there is no evidence that these keys are defined in the localization files. This suggests that the localization keys might be missing.
- Files to check for missing localization keys:
src/panels/config/devices/device-detail/integration-elements/zwave_js/ha-device-info-zwave_js.ts
src/panels/config/integrations/integration-panels/zwave_js/zwave_js-provisioned.ts
src/panels/config/integrations/integration-panels/zwave_js/dialog-zwave_js-add-node.ts
Analysis chain
Ensure that the security class names are correctly localized.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify localization keys for security classes.
# Test: Search for the localization keys in the localization files. Expect: Keys should exist.
rg --type yaml 'ui.panel.config.zwave_js.security_classes'
Length of output: 58
Script:
#!/bin/bash
# Description: Verify localization keys for security classes in the entire codebase.
# Test 1: Search for the localization keys in all YAML files. Expect: Keys should exist.
rg --type yaml 'security_classes'
# Test 2: Search for the usage of localization keys in the codebase. Expect: Keys should be used in the code.
rg 'ui.panel.config.zwave_js.security_classes'
Length of output: 1021
Breaking change
Proposed change
As a result of bumping zwave-js-server-python in core, we learned that the API outputs and inputs didn't map well to the zwave-js model. The primary change is that we've switched from camel case to snake for some of the provisioning APIs and need to account for that here. Interestingly enough, the type for QRProvisioningInformation was previously wrong and matches the new key names.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
These changes enhance code clarity and consistency, providing a smoother experience for developers and maintainers.