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

Implement handle_set_time_request in TuyaMCUCluster #1737

Merged
merged 8 commits into from Sep 26, 2022

Conversation

javicalle
Copy link
Collaborator

Fully implement the handle_set_time_request in TuyaMCUCluster.

The code has been taken from the TuyaManufCluster and must have the same behavior that the already supported devices (set_time_offset and set_time_local_offset):

Related to: #1702

All credit goes to @RhavoX who perform the first code and tests.

Fully implement the `handle_set_time_request` in `TuyaMCUCluster`.

The code has been taken from the `TuyaManufCluster` and must have the same behavior that the already supported devices (`set_time_offset` and `set_time_local_offset`):
* https://github.com/zigpy/zha-device-handlers/blob/dev/zhaquirks/tuya/__init__.py#L377-L398
@RhavoX
Copy link

RhavoX commented Sep 8, 2022

@javicalle hi, isn't the manufacturer=foundation.ZCLHeader.NO_MANUFACTURER_ID param missing here?

@javicalle
Copy link
Collaborator Author

isn't the manufacturer=foundation.ZCLHeader.NO_MANUFACTURER_ID param missing here?

Although I am becoming more and more certain that Tuya is not using the manufacturer code in any case, the implementation according to the specifications should use it. That's why we don't override it here.

The cases where it has been detected that it is necessary to override it, we do it with the NO_MANUFACTURER_ID, but it is done in a specific class. Like here:

It can be confusing to review the implementations because there are 2 architectures implementing the quirks. I am taking care of the one that uses the TuyaMCUCluster

When I can fix the tests I will review your case and tell you how it would be implemented.

@javicalle
Copy link
Collaborator Author

I don't know why test coverage is failing.
Tests has been added and in my local environment the coverage for tuya classes are better than here:

zhaquirks/tuya/__init__.py                        551    152    72%
zhaquirks/tuya/air/__init__.py                     18      0   100%
zhaquirks/tuya/air/ts0601_air_quality.py           11      0   100%
zhaquirks/tuya/mcu/__init__.py                    179      0   100%
zhaquirks/tuya/ts000x.py                           18      0   100%

🤷🏻‍♂️

@javicalle javicalle closed this Sep 10, 2022
@javicalle javicalle reopened this Sep 10, 2022
@coveralls
Copy link

coveralls commented Sep 10, 2022

Pull Request Test Coverage Report for Build 3123440246

  • 4 of 15 (26.67%) changed or added relevant lines in 1 file are covered.
  • 11 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 71.504%

Changes Missing Coverage Covered Lines Changed/Added Lines %
zhaquirks/tuya/mcu/init.py 4 15 26.67%
Files with Coverage Reduction New Missed Lines %
zhaquirks/tuya/mcu/init.py 11 51.4%
Totals Coverage Status
Change from base Build 2972024041: -0.06%
Covered Lines: 5277
Relevant Lines: 7380

💛 - Coveralls

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2022

Codecov Report

Base: 71.56% // Head: 71.50% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (83ef4df) compared to base (e5ba53f).
Patch coverage: 26.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1737      +/-   ##
==========================================
- Coverage   71.56%   71.50%   -0.07%     
==========================================
  Files         239      239              
  Lines        7358     7380      +22     
==========================================
+ Hits         5266     5277      +11     
- Misses       2092     2103      +11     
Impacted Files Coverage Δ
zhaquirks/tuya/mcu/__init__.py 51.68% <26.66%> (-2.31%) ⬇️
zhaquirks/tuya/ts0043.py 100.00% <0.00%> (ø)
zhaquirks/tuya/ts0601_trv.py 33.21% <0.00%> (ø)
zhaquirks/tuya/ts011f_plug.py 100.00% <0.00%> (ø)
zhaquirks/tuya/ts0601_cover.py 100.00% <0.00%> (ø)
zhaquirks/tuya/air/ts0601_air_quality.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@javicalle
Copy link
Collaborator Author

I desist from trying to fix the coverage, although I don't know if that could be a problem in future PRs (if this PR is merged without complying with, any future PRs, it will not comply). I can't understand how can the coverage be lower than before, or why complain about code that in my local environment is covereded.

I'm going to try to review the tests of the Tuya devices in another PR because I don't see the point in continuing to add tests that are not related to the PR.

@dmulcahey
Copy link
Collaborator

I’ll take a peek in a bit.

@javicalle javicalle closed this Sep 25, 2022
@javicalle javicalle reopened this Sep 25, 2022
@javicalle
Copy link
Collaborator Author

Now all the checks are OK.
For me is ready to go.

@javicalle javicalle added the needs final review PR is ready for a final review from a maintainer label Sep 25, 2022
@dmulcahey dmulcahey merged commit 412a1f1 into zigpy:dev Sep 26, 2022
@javicalle
Copy link
Collaborator Author

Thanks David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs final review PR is ready for a final review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants