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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache device class data #958

Merged
merged 10 commits into from
Apr 26, 2024
Merged

Conversation

raman325
Copy link
Contributor

bdraco shared this profile with me
image

We don't need to create the DeviceClass instance every time which will reduce the calls significantly. We also created DeviceClassItem on each lookup so I've stored it. If anything changes we will create a new DeviceClass instance on update so we shouldn't be storing any stale data in this PR

MartinHjelmare
MartinHjelmare previously approved these changes Apr 25, 2024
Copy link
Contributor

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare
Copy link
Contributor

There's a merge conflict.

@raman325 raman325 dismissed MartinHjelmare’s stale review April 26, 2024 02:40

bigger change than I thought due to a branching issue

zwave_js_server/model/endpoint.py Outdated Show resolved Hide resolved
zwave_js_server/model/node/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Can we test the uncovered lines?

raman325 and others added 4 commits April 26, 2024 02:44
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@raman325 raman325 merged commit d101500 into home-assistant-libs:master Apr 26, 2024
1 check passed
@raman325 raman325 deleted the device_class branch April 26, 2024 21:48
@raman325
Copy link
Contributor Author

whoops sorry @MartinHjelmare didn't mean to merge this yet but hope this addressed all the concerns

@MartinHjelmare
Copy link
Contributor

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants