-
Notifications
You must be signed in to change notification settings - Fork 356
fix!: remove unnecessary throwing #1455
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
Conversation
CodSpeed Performance ReportMerging #1455 will improve performances by 98.36%Comparing Summary
Benchmarks breakdown
|
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.
Pull Request Overview
This PR removes unnecessary exception throwing from the getAttribute method, changing it from a throwing function to one that returns undefined for invalid attributes. This change improves performance by eliminating flow control via exceptions and removes duplicate logic that was previously handled by the hasAttribute method.
- Changed
getAttributeto returnAttribute | undefinedinstead of throwing for invalid attributes - Removed the
hasAttributemethod that was previously used to avoid exceptions - Updated all callers to handle undefined return values and log warnings for invalid attributes in read operations
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/zspec/zcl/utils.test.ts | Updated tests to expect undefined instead of thrown exceptions for invalid attributes |
| test/zspec/zcl/frame.test.ts | Changed null parameters to undefined for consistency |
| test/zcl.test.ts | Updated attribute checking tests and improved variable naming |
| test/controller.test.ts | Refactored frame creation to use Zcl.Frame.create and added null checks for attributes |
| src/zspec/zcl/utils.ts | Removed throwing behavior and hasAttribute method from cluster implementation |
| src/zspec/zcl/definition/tstype.ts | Updated Cluster interface to reflect new getAttribute signature |
| src/zspec/zcl/buffaloZcl.ts | Updated attribute lookup logic to handle undefined returns |
| src/controller/model/group.ts | Added null checks and warning logging for invalid attributes |
| src/controller/model/endpoint.ts | Added comprehensive null checks throughout attribute access patterns |
| src/controller/model/device.ts | Updated custom read response logic to handle undefined attributes |
| src/controller/helpers/zclFrameConverter.ts | Simplified attribute name resolution using nullish coalescing |
| package.json | Updated vitest dependency version |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
test/controller.test.ts:7265
- Debug console.log statement should be removed from test code as it adds unnecessary output during test execution.
});
|
Corrected a flaw in the benchmarking, so last edit from codspeed report is not representative (previous edit is the right one, albeit with the flaw). |
|
For future reference, this PR has the following breaking changes:
|
Throwing on
getAttributewas used mostly for flow control, not great for perf. Also, most of the time, that was bypassed by the use ofhasAttribute(now removed), which resulted in unnecessary dupe get logic.Invalid attributes for endpoint/group read are now ignored instead of failing the whole request. Added some logging for it, and test coverage (wasn't covered before when it was throwing since the change didn't fail tests).
Note: will require checking a couple of places where
getAttributeis used in ZHC & Z2M (will at least fail typing).Also:
Zcl.Frame.createinstead of writing the whole object by hand).