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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add enhancements to qBittorrent sensor #42837

Closed
wants to merge 1 commit into from
Closed

Add enhancements to qBittorrent sensor #42837

wants to merge 1 commit into from

Conversation

EverythingSmartHome
Copy link

@EverythingSmartHome EverythingSmartHome commented Nov 4, 2020

Proposed change

Added enhancements to the qBittorrent sensor which gives the ability to the value of each type of torrent into a sensor (total_torrents, active_torrents, inactive_torrents, seeding_torrents, paused_torrents, downloading_torrents, resumed_torrents, upload_speed, download_speed, current_status) and for each sensor it will also provide the list of each torrent in the attribute, and report the progress.

Type of change

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

Example entry for configuration.yaml:

# Example configuration.yaml
sensor:
  - platform: qbittorrent
    url: "http://ip_address_or_hostname:8080"
    username: admin
    password: password
    monitored_conditions:
      - total_torrents
      - active_torrents
      - inactive_torrents
      - seeding_torrents
      - paused_torrents
      - downloading_torrents
      - resumed_torrents
      - upload_speed
      - download_speed
      - current_status

Additional information

There should be no breaking changes if "monitored_conditions" variable is not specified in the config, it will default to the way qBittorrent is setup now only monitoring the current 3 values - "Current status", "Download Speed" and "Upload Speed"

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • 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:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@homeassistant
Copy link
Contributor

Hi @EverythingSmartHome,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

download = data["server_state"]["dl_info_speed"]
upload = data["server_state"]["up_info_speed"]

if self.type == SENSOR_TYPE_CURRENT_STATUS:
if upload > 0 and download > 0:
self._state = "up_down"
self.attrs = attributes
Copy link
Contributor

@Adminiuga Adminiuga Nov 5, 2020

Choose a reason for hiding this comment

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

Can you just move this line out side of these if statements and just do it at the end once?

Comment on lines +60 to +62
vol.Optional(CONF_MONITORED_CONDITIONS, default=DEFAULT_CONDITIONS): vol.All(
cv.ensure_list, [vol.In(SENSOR_TYPES)]
),
Copy link
Member

Choose a reason for hiding this comment

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

Adding this goes against ADR-0003:

We don't merge PRs with data selectors. We remove existing unneeded selectors until Home Assistant 1.0. We focus on a good view/frontend to select and group available data.

Choose a reason for hiding this comment

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

Ah OK I hadn't seen that, I saw here about the use of MONITORED_CONDITIONS and assumed it was OK as didn't see any mention there of any issues regarding using.

Can you make any suggestions as to what to change? Sorry if it's a basic question, this is already way over my head!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, monitored conditions is still used in some integrations. Ideally you would add a config flow so users can set it up via the frontend and do not have to alter their YAML. With a config flow a user can also easily disable sensors that they do not want to show on the frontend.

However, in this case I think it would be okay to just remove the monitored conditions and just set up all of them automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi springstan, I've added everythingsmarthomes to my PR and extended the qbittorrent integration with flow and added some services #43661

Dev automation moved this from Needs review to Review in progress Nov 6, 2020
Copy link
Member

@springstan springstan left a comment

Choose a reason for hiding this comment

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

Hope this helps 鉁岋笍

@geoffreylagaisse
Copy link
Contributor

I have updated the qbittorent component with some goodies to flow, but i'm not getting my unittest to work.
#43661

Anybody some advise?

@github-actions
Copy link

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.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 26, 2021
@github-actions github-actions bot closed this Feb 2, 2021
Dev automation moved this from Review in progress to Cancelled Feb 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

5 participants