feat: runner s11 integration

YogevBokobza commented 3 months ago

Description

BREAKING CHANGES:

SwitcherApi require new parameter - device_type
Shutter API "stop" replaced with "stop_shutter".

Related issue (if any): fixes #issue_number_goes_here

Checklist

[x] I have followed this repository's contributing guidelines.
[x] I will adhere to the project's code of conduct.

Additional information

Anything else?

github-actions[bot] commented 3 months ago

Test Results

246 tests 246 :white_check_mark: 2s :stopwatch: 1 suites 0 :zzz: 1 files 0 :x:

Results for commit 2c4fdd8d.

:recycle: This comment has been updated with latest results.

codecov[bot] commented 3 months ago

Codecov Report

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

Project coverage is 97.67%. Comparing base (32e8073) to head (2c4fdd8). Report is 1 commits behind head on dev.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## dev #780 +/- ## ========================================== - Coverage 98.52% 97.67% -0.86% ========================================== Files 11 11 Lines 1087 1289 +202 ========================================== + Hits 1071 1259 +188 - Misses 16 30 +14 ```

YogevBokobza commented 3 months ago

@thecode @TomerFi I think we are finally ready to merge this S11 integration.. I don't think anything is left.. I tested everything from my end and it looks like its working.. I will do a few more tests while you are reviewing it.

YogevBokobza commented 3 months ago

@TomerFi Approve to merge?

TomerFi commented 3 months ago

@TomerFi Approve to merge?

I prefer to take a look before, to make sure we didn't miss anything.

YogevBokobza commented 3 months ago

@TomerFi Approve to merge?

I prefer to take a look before, to make sure we didn't miss anything.

Please finish your review soon.. I already started to make progress in implementing those changes to the HA as seen here: https://github.com/home-assistant/core/pull/122404 The current code tested working perfectly in HA controlling my devices which are Switcher Runner Mini, Switcher Runner S11, Switcher Mini, Switcher Power Plug, Switcher Breeze

YogevBokobza commented 3 months ago

@TomerFi Approve to merge?

I prefer to take a look before, to make sure we didn't miss anything.

Please finish your review soon.. I already started to make progress in implementing those changes to the HA as seen here: home-assistant/core#122404 The current code tested working perfectly in HA controlling my devices which are Switcher Runner Mini, Switcher Runner S11, Switcher Mini, Switcher Power Plug, Switcher Breeze

Made progress here too: https://github.com/TomerFi/switcher_webapi/pull/715

TomerFi commented 3 months ago

@YogevBokobza Yep, sorry I was away. Let's merge this! We need the title of the PR to start with "feat" and the body of the PR to include "BREAKING CHANGES:" and will include a list of what we broke, i.e. "API not takes type." These will affect the version bump.

YogevBokobza commented 3 months ago

@TomerFi @thecode I changed the title and fixed the description.

I also opened this PR with small fixes. We can merge this PR and after that the other one or vice versa.. Your choice..

thecode commented 3 months ago

@YogevBokobza

BREAKING CHANGES:

API does not take type (device_type)..

Can you explain this?

If calling Shutter or Light operations with a Switcher Runner S11 device, a token must be specified.

This is a new device so it is not under breaking changes, it will not break anything for existing users.

self._login function no longer called with DeviceType parameter, now only self._login()

This is an internal change, self._login is not exposed to users and should not be listed under breaking changes.

YogevBokobza commented 3 months ago

@YogevBokobza

BREAKING CHANGES:

API does not take type (device_type)..

Can you explain this?

If calling Shutter or Light operations with a Switcher Runner S11 device, a token must be specified.

This is a new device so it is not under breaking changes, it will not break anything for existing users.

self._login function no longer called with DeviceType parameter, now only self._login()

This is an internal change, self._login is not exposed to users and should not be listed under breaking changes.

I removed the last 2 from the breakchanges.. Regarding the first one here is an example: https://github.com/TomerFi/aioswitcher/pull/780/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R42 now need to supply the device_type

thecode commented 3 months ago

I removed the last 2 from the breakchanges.. Regarding the first one here is an example: https://github.com/TomerFi/aioswitcher/pull/780/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R42 now need to supply the device_type

You wrote "API does not take type (device_type)", but the change introduce a new parameter device_type. The message should be: SwitcherApi require new parameter - device_type

YogevBokobza commented 3 months ago

I removed the last 2 from the breakchanges.. Regarding the first one here is an example: https://github.com/TomerFi/aioswitcher/pull/780/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R42 now need to supply the device_type

You wrote "API does not take type (device_type)", but the change introduce a new parameter device_type. The message should be: SwitcherApi require new parameter - device_type

Accepted and replaced.

TomerFi commented 3 months ago

@thecode @YogevBokobza Can I merge?

TomerFi commented 3 months ago

Merging https://github.com/TomerFi/aioswitcher/pull/782 first!

YogevBokobza commented 3 months ago

@thecode @YogevBokobza Can I merge?

@TomerFi I think we can merge.. @thecode already approved this PR before the last PR merge and approved the last PR

TomerFi commented 3 months ago

I assume you're right, still, I prefer to wait for @thecode to approve again because I noticed some related discussion you guys had in another issue.

YogevBokobza commented 3 months ago

I assume you're right, still, I prefer to wait for @thecode to approve again because I noticed some related discussion you guys had in another issue.

This PR is not related to the issue mentioned. The logic here for the Switcher Runner (normal) have not been changed. For my Switcher Runner devices (Mini, S11) it's working. The Switcher Runner Mini has the same logic as Switcher Runner. So no change here.

My guess with his issue is old firmware of his Switcher device or his device itself have some kind of problem.

thecode commented 3 months ago

My guess with his issue is old firmware of his Switcher device or his device itself have some kind of problem.

This PR can be merged, but I prefer we fix the Runner before a new release. The firmware version of his device is not related.

TomerFi commented 3 months ago

This PR can be merged, but I prefer we fix the Runner before a new release. The firmware version of his device is not related.

Merged. It has not been released yet. I assume https://github.com/TomerFi/aioswitcher/issues/783 is the issue in question.

@YogevBokobza @thecode Thank you both for this fantastic work and collaboration; I apologize for being absent at times. ❤️

TomerFi / aioswitcher