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

Extend tumble dryer support #22

Merged
merged 10 commits into from
Oct 13, 2021
Merged

Extend tumble dryer support #22

merged 10 commits into from
Oct 13, 2021

Conversation

terminet85
Copy link
Contributor

@terminet85 terminet85 commented Oct 12, 2021

I’ve extended support to tumble dryer and it translation.

@@ -115,15 +118,28 @@ class TumbleDryerStatus:
program: int
remaining_minutes: int
remote_control: bool
dry_level: int
refresh: bool
Copy link
Owner

Choose a reason for hiding this comment

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

What is the meaning of this bool value? Is this an extra feature on top of programs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a tumble dryer features. There are more dry level (as ready to be ironing or to be put in the closet) and when the machine is running reach step by step these level. You can choose to pull out some clothes (as shirts that need to be ironed) and leave t-shirt to be so dry to fold in the closet, for example.

Refresh is a feature that you can turn on to keep machine running time-by-time, so clothes don't crease.

Copy link
Owner

@ofalvai ofalvai left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, it's really cool :) I had a couple of comments about naming things, I want to fully understand what each field means so that I can touch the code later without having to buy a tumble dryer :)
Oh and there is one failing test case now that the sensor attributes have changed. Your pull request didn't trigger the GitHub workflow that runs the tests, but I think I fixed the triggers on the main branch. It should work if you pull and merge the main branch and push the commit to this PR.

@terminet85
Copy link
Contributor Author

Thank you for the contribution, it's really cool :) I had a couple of comments about naming things, I want to fully understand what each field means so that I can touch the code later without having to buy a tumble dryer :) Oh and there is one failing test case now that the sensor attributes have changed. Your pull request didn't trigger the GitHub workflow that runs the tests, but I think I fixed the triggers on the main branch. It should work if you pull and merge the main branch and push the commit to this PR.

You're welcome. I've just fetched from upstream, should be ok. Let me know if you need more changes or infos.

@ofalvai
Copy link
Owner

ofalvai commented Oct 13, 2021

Thank you, the changes look good, but there is still one failing test case.

@terminet85
Copy link
Contributor Author

Thank you, the changes look good, but there is still one failing test case.

I cannot find what it's wrong. Any advice?

@ofalvai
Copy link
Owner

ofalvai commented Oct 13, 2021

        assert state
        assert state.state == "Idle"
>       assert state.attributes == {
            'program': 1,
            'remaining_minutes': 150,
            'remote_control': True,
            'friendly_name': 'Tumble dryer',
            'icon': 'mdi:tumble-dryer'
        }

A test case in tests/test_sensor_tumble_dryer.py is failing because you added more attributes to the sensor.

@terminet85
Copy link
Contributor Author

I've added the missing attributes but you need to approve my commit in order to pass the test I guess.

@ofalvai
Copy link
Owner

ofalvai commented Oct 13, 2021

It's still not passing the test case for some reason, I recommend running the tests locally to find out more. Activate the Python virtual env, then pip install -r requirements_test.txt, then py.test tests

@terminet85
Copy link
Contributor Author

Finally fix'd

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #22 (adc2e36) into main (68ee0f5) will increase coverage by 1.32%.
The diff coverage is 92.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   86.66%   87.99%   +1.32%     
==========================================
  Files           6        6              
  Lines         405      458      +53     
==========================================
+ Hits          351      403      +52     
- Misses         54       55       +1     
Impacted Files Coverage Δ
custom_components/candy/client/model.py 80.00% <75.00%> (+1.73%) ⬆️
custom_components/candy/sensor.py 98.08% <97.56%> (-0.13%) ⬇️
custom_components/candy/const.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68ee0f5...adc2e36. Read the comment docs.

@ofalvai
Copy link
Owner

ofalvai commented Oct 13, 2021

Awesome, thank you so much for the contribution!

@ofalvai ofalvai merged commit ec5a26b into ofalvai:main Oct 13, 2021
@ofalvai ofalvai mentioned this pull request Oct 13, 2021
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