core icon indicating copy to clipboard operation
core copied to clipboard

Add QBittorent switch to control alternative speed

Open Sebclem opened this issue 1 year ago • 6 comments

Proposed change

This PR add a switch to the QBittorent integration that control the 'alternative speed'. I have also fixed the current_status key in string.json

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

I was not sure to go the "switch" way as QBittorrent only expose 'toggle' and no 'turn on' et 'turn off'. I had first thinking exposing a binary sensor that represent the sate of the alternative speed and a button to toggle it. So let me know if you prefer this approach.

This PR is waiting for this pull request to be merged in the Python-QBittorent lib: https://github.com/v1k45/python-qBittorrent/pull/66

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [x] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [x] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [x] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Sebclem avatar Jan 09 '24 10:01 Sebclem

Hey there @geoffreylagaisse, @finder39, mind taking a look at this pull request as it has been labeled with an integration (qbittorrent) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of qbittorrent can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign qbittorrent Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Jan 09 '24 10:01 home-assistant[bot]

Hey @finder39 and @geoffreylagaisse, I'm taking a little look around and, can we consider switching to qbittorrent-api ? It's seams to be much more used and the code seams to be well tested. It's seams that it's more maintained too.

And this lib also expose a method called transfer_set_speed_limits_mode witch it's more clean that using my dirty hack from this PR.

Sebclem avatar Jan 09 '24 12:01 Sebclem

Hey @finder39 and @geoffreylagaisse, I'm taking a little look around and, can we consider switching to qbittorrent-api ? It's seams to be much more used and the code seams to be well tested. It's seams that it's more maintained too.

And this lib also expose a method called transfer_set_speed_limits_mode witch it's more clean that using my dirty hack from this PR.

I could be down to swap it once we take a longer look at it. I think it'd be nice to get this PR for get_torrents service landed first and get it to a good stable place before changing the underlying server connection

finder39 avatar Jan 09 '24 19:01 finder39

Overall this looks like a great addition! Please go through and fix up some of the comments. In addition, please add tests where necessary (such as for the functions).

I will take a look at this tomorrow 👍

Sebclem avatar Jan 09 '24 20:01 Sebclem

I could be down to swap it once we take a longer look at it. I think it'd be nice to get this PR for get_torrents service landed first and get it to a good stable place before changing the underlying server connection

I will wait for your PR to be merged as this one is blocked until my PR is merged on the currently used lib or we completely change the lib.

Sebclem avatar Jan 09 '24 20:01 Sebclem

Any update for this PR. I am waiting for this with great anxiety XD...

alorle avatar Feb 05 '24 23:02 alorle

@finder39 Applied all the change, can you take another quick review ?

My PR is still pending on the lib repo, I think we can sadly say that this lib is unmaintained...

Sebclem avatar Mar 07 '24 16:03 Sebclem

@finder39 Applied all the change, can you take another quick review ?

My PR is still pending on the lib repo, I think we can sadly say that this lib is unmaintained...

Approved with 1 comment. I am traveling right now without my laptop so I can't test it, but trust that all is working. Great addition!

Yeah, I'm down to swap the underlying library. I was hoping the wait til the my PR for the service call went up but I think things have stalled there. So feel free to put a diff up that changes that if you wish.

finder39 avatar Mar 08 '24 14:03 finder39

@finder39 Applied all the change, can you take another quick review ? My PR is still pending on the lib repo, I think we can sadly say that this lib is unmaintained...

Approved with 1 comment. I am traveling right now without my laptop so I can't test it, but trust that all is working. Great addition!

Yeah, I'm down to swap the underlying library. I was hoping the wait til the my PR for the service call went up but I think things have stalled there. So feel free to put a diff up that changes that if you wish.

Ok, i'll open a new PR with this change :+1:

Sebclem avatar Mar 08 '24 14:03 Sebclem

The required changes to the qbitorrent library were merged

bwrsandman avatar Mar 18 '24 03:03 bwrsandman

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!

github-actions[bot] avatar May 17 '24 04:05 github-actions[bot]

Still waiting for #112394

Sebclem avatar May 17 '24 05:05 Sebclem

@Sebclem #112394 has been merged.

frenck avatar Jun 09 '24 12:06 frenck

@Sebclem #112394 has been merged.

Sorry, I linked the wrong pr, this is the #113394

Sebclem avatar Jun 09 '24 12:06 Sebclem

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Jul 07 '24 21:07 home-assistant[bot]

Thank you for your review @emontnemery 👍

Sebclem avatar Jul 30 '24 09:07 Sebclem