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 common interface for vacuums #1368

Merged
merged 20 commits into from
Apr 25, 2022
Merged

Conversation

2pirko
Copy link
Contributor

@2pirko 2pirko commented Mar 20, 2022

Created first simple VaccumDevice interface

@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2022

Codecov Report

Merging #1368 (01406ea) into master (efe68e0) will increase coverage by 0.01%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #1368      +/-   ##
==========================================
+ Coverage   84.17%   84.18%   +0.01%     
==========================================
  Files         133      135       +2     
  Lines       13330    13347      +17     
  Branches     1484     1485       +1     
==========================================
+ Hits        11220    11236      +16     
- Misses       1899     1900       +1     
  Partials      211      211              
Impacted Files Coverage Δ
miio/interfaces/vacuuminterface.py 90.00% <90.00%> (ø)
...io/integrations/vacuum/dreame/dreamevacuum_miot.py 76.35% <100.00%> (+0.08%) ⬆️
miio/integrations/vacuum/mijia/g1vacuum.py 74.32% <100.00%> (+0.17%) ⬆️
miio/integrations/vacuum/roborock/vacuum.py 63.23% <100.00%> (+0.07%) ⬆️
...io/integrations/vacuum/roidmi/roidmivacuum_miot.py 89.48% <100.00%> (+0.03%) ⬆️
miio/integrations/vacuum/viomi/viomivacuum.py 57.88% <100.00%> (+0.11%) ⬆️
miio/interfaces/__init__.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@rytilahti
Copy link
Owner

rytilahti commented Mar 23, 2022

Hi @2pirko, take a look at my alternative proposal #1373. It is based on typing.Protocol, so it is not necessary to inherit from the interface class, although doing so helps to spot the mistakes.

@2pirko
Copy link
Contributor Author

2pirko commented Mar 23, 2022

Hi @2pirko, take a look at my alternative proposal #1373. It is based on typing.Protocol, so it is not necessary to inherit from the interface class, although doing so helps to spot the mistakes.

I do not have any experience with Protocol, so I cannot comment, if there is any advantage compare to abstract class and inheritance. From documentation it seems Protocol is designed for run-time checks, while inheritance could be better for statically written code, helping to ensure, all abstract methods are implemented.

@2pirko
Copy link
Contributor Author

2pirko commented Apr 5, 2022

@rytilahti , could you please approve or reject the pull request? So I know whether it makes sense to continue on that.

@rytilahti
Copy link
Owner

From documentation it seems Protocol is designed for run-time checks, while inheritance could be better for statically written code, helping to ensure, all abstract methods are implemented.

Correct. I was just thinking about not requiring all integrations to add all potential methods that should be a part of the common API. On hindsight, I think your proposal makes makes more sense. I.e., the abstract base class should provide the basic features that ought to be supported, and maybe extend that using protocols for extra features that can be implemented if the device supports such.

So let's proceed with your proposal with the following changes:

  1. Move the abstract VacuumInterface to miio/interfaces/vacuuminterface.py. I'm not completely sure if it should be simply called Vacuum or include the interface word on it, but that can be clarified later on.
  2. Use multi-inheritance to enforce the interface in implementation classes. This avoids coupling the interface with the underlying "transport" (miio/miot) -- vacuum implementations using miot for communication should inherit from both MiotDevice and from the vacuum interface.

@2pirko 2pirko changed the title Vaccum device Vaccum interface Apr 7, 2022
@2pirko
Copy link
Contributor Author

2pirko commented Apr 7, 2022

Ready for merge

@2pirko
Copy link
Contributor Author

2pirko commented Apr 20, 2022

@rytilahti , could you merge this one? I'll extend the API in next pull request.

@2pirko
Copy link
Contributor Author

2pirko commented Apr 22, 2022

@rytilahti any updates?

@2pirko
Copy link
Contributor Author

2pirko commented Apr 24, 2022

@rytilahti, could you merge this one? I'll extend the API in next pull request.

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.

Looks good to me, just a minor nitpick wrt. the exported symbols and then this is good to go.

Btw, you can always use different code branches as a base branch for any future developments to avoid deadlocks when code reviews are pending :-)

miio/integrations/vacuum/dreame/dreamevacuum_miot.py Outdated Show resolved Hide resolved
@2pirko
Copy link
Contributor Author

2pirko commented Apr 25, 2022

@rytilahti , ready for merge again

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.

Looks great, thanks for working on this @2pirko! The CI issue is unrelated so let's merge this.

@rytilahti rytilahti changed the title Vaccum interface Add common interface for vacuums Apr 25, 2022
@rytilahti rytilahti merged commit 009012c into rytilahti:master Apr 25, 2022
@2pirko
Copy link
Contributor Author

2pirko commented Apr 25, 2022

Haleluja!!!!!

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