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

Add multi map handling to roborock #1596

Merged
merged 23 commits into from
Dec 5, 2022
Merged

Conversation

starkillerOG
Copy link
Contributor

Split out from #1543

Add basic multi map support to roborock vacuums

  • Get current map
  • Set current map

Later PR will add clean details per floor and floor information to last clean details.

@starkillerOG
Copy link
Contributor Author

@rytilahti ready for review/merge

@starkillerOG
Copy link
Contributor Author

@rytilahti thanks for the quick revieuws the last few days, really making great progress!
Can you also review/merge this, so I can continue with the last part of this (the per floor cleaning details).

@starkillerOG
Copy link
Contributor Author

@rytilahti could you have a look at this and merge?
This is the straight forward part of the multi map support.

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

I have had these pending reviews for a while, but didn't want to send them before offering a solution for some of them. Considering I'm busy with other things at the moment, I thought I'll just send the feedback as-is. Please take a look @starkillerOG and let me know what you think.

Also, you could create a PR for the part 2 (or merge with this?), if you think it will make it simpler to review the concepts.

miio/integrations/vacuum/roborock/vacuumcontainers.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuumcontainers.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
@starkillerOG
Copy link
Contributor Author

@rytilahti I finished processing all feedback, can you have another look?

@starkillerOG
Copy link
Contributor Author

@rytilahti All checks pass now, the error is unrelated.
Part 2 of the mult map support is a bit more difficult, it is basically what is left in #1543.

But because part 2 builds upon part 1 I will get lots of merge conflicts when we change stuff in part 1.
I don't want to make one big PR because that will take a lot longer to review.

I think this part 1 is really straight forward and can be merged quite fast.
Lets get this merged now and then I will submit part 2.

@starkillerOG
Copy link
Contributor Author

@rytilahti could you look at my responses/merge?

miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuumcontainers.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuum.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2022

Codecov Report

Merging #1596 (816b5a0) into master (27cc3ad) will decrease coverage by 0.00%.
The diff coverage is 78.33%.

@@            Coverage Diff             @@
##           master    #1596      +/-   ##
==========================================
- Coverage   80.70%   80.69%   -0.01%     
==========================================
  Files         157      157              
  Lines       15420    15480      +60     
  Branches     3427     3443      +16     
==========================================
+ Hits        12445    12492      +47     
- Misses       2719     2731      +12     
- Partials      256      257       +1     
Impacted Files Coverage Δ
miio/integrations/vacuum/roborock/vacuum.py 61.47% <45.45%> (-0.74%) ⬇️
...o/integrations/vacuum/roborock/vacuumcontainers.py 84.82% <96.29%> (+0.90%) ⬆️
.../integrations/vacuum/roborock/tests/test_vacuum.py 98.38% <100.00%> (+0.10%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@starkillerOG
Copy link
Contributor Author

@rytilahti I have finished processing your feedback, thanks!
Do you have any additional comments?

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

This is good to go as soon as the CI passes, thanks @starkillerOG! 👍 edit: could you please add tests that MapList is parsed properly (and property accesses return what's expected)?

@rytilahti rytilahti changed the title Add Multi map to roborock part 1 Add multi map handling to roborock Dec 5, 2022
@starkillerOG
Copy link
Contributor Author

starkillerOG commented Dec 5, 2022

@rytilahti I added the test you suggested, how can I easily run the test locally?
(I think you have a poetry command for that right)?

pytest miio\integrations\vacuum\roborock can be used to run the tests locally

@starkillerOG
Copy link
Contributor Author

@rytilahti I included the test you requested, and it is now passing.
Can you merge?

@rytilahti
Copy link
Owner

rytilahti commented Dec 5, 2022

The codecov still fails due to decreased coverage, but as some other parts are not covered either I'll let it slip this time, thanks again! Btw, if you click on the codecov links you can see what parts were covered and what not: https://app.codecov.io/gh/rytilahti/python-miio/pull/1596

@rytilahti rytilahti merged commit 0d0e891 into rytilahti:master Dec 5, 2022
@starkillerOG
Copy link
Contributor Author

Thanks, I will start working on the final part of the multi maps handeling

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

Successfully merging this pull request may close these issues.

3 participants