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

Cluster.bind is not async #3704

Closed
wants to merge 2 commits into from
Closed

Conversation

RubenVerborgh
Copy link

Proposed change

Cluster.bind is not defined as async, yet CustomCluster defines it as such. During the development of #3701, I found this to lead to unpredictable behavior where bind was not consistently called by ZHA. Pylint also calls this out:

% pylint zhaquirks/__init__.py
************* Module zhaquirks
zhaquirks/__init__.py:74:4: W0236: Method 'bind' was expected to be 'non-async', found it instead as 'async' (invalid-overridden-method)

Additional information

Checklist

  • The changes are tested and work correctly
  • pre-commit checks pass / the code has been formatted using Black
  • Tests have been added to verify that the new code works

Copy link

codecov bot commented Jan 12, 2025

Codecov Report

Attention: Patch coverage is 62.16216% with 14 lines in your changes missing coverage. Please review.

Project coverage is 89.87%. Comparing base (57bcec8) to head (94f7ec8).
Report is 15 commits behind head on dev.

Files with missing lines Patch % Lines
zhaquirks/__init__.py 60.00% 2 Missing ⚠️
zhaquirks/danfoss/thermostat.py 66.66% 2 Missing ⚠️
zhaquirks/osram/lightifyx4.py 33.33% 2 Missing ⚠️
zhaquirks/tuya/ts0601_valve.py 33.33% 2 Missing ⚠️
zhaquirks/xiaomi/aqara/opple_remote.py 33.33% 2 Missing ⚠️
zhaquirks/xiaomi/aqara/tvoc.py 33.33% 2 Missing ⚠️
zhaquirks/develco/air_quality.py 50.00% 1 Missing ⚠️
zhaquirks/waxman/leaksmart.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #3704   +/-   ##
=======================================
  Coverage   89.86%   89.87%           
=======================================
  Files         322      322           
  Lines       10387    10386    -1     
=======================================
  Hits         9334     9334           
+ Misses       1053     1052    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheJulianJES TheJulianJES added the needs review This PR should be reviewed soon, as it generally looks good. label Jan 12, 2025
@TheJulianJES
Copy link
Collaborator

Nice catch! Interesting this wasn't noticed for so long..

I think bind in Cluster should be marked as async in zigpy though, but I'll check this with the others. (cc @puddly)
We await bind() in ZHA, as it's a normal request.

ZDO.bind() will execute ZDO.__getattr__. That then calls ZDO.request which finally calls
(async) Device.request, so we do have to await this in the end.

The ZDO stuff seems to be unchanged since quite a while, while other parts of zigpy were refactored. This code could be cleaner IMO and some methods should just be marked as async I think (e.g. ZDO.request and likely most others).

@RubenVerborgh
Copy link
Author

Thanks @TheJulianJES, I agree with your assessment and opened zigpy/zigpy#1533 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review This PR should be reviewed soon, as it generally looks good.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants