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

Refactor nzbget to support future platform changes #26462

Merged
merged 15 commits into from Sep 15, 2019

Conversation

@chriscla
Copy link
Contributor

commented Sep 6, 2019

Breaking Change:

The NZBGet integration has been changed to support multiple platforms and future events, and common code has been centralized to the component. The configuration has moved from the sensor platform to the nzbget key in configuration.yaml, and the monitored_variables option has been removed. Users need to update their configuration.

Description:

Re-factor NZBGet component to move core logic out of the sensor platform and into a common component. Update config to have a nzbget: root-level entry.

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io/pull/10300

Example entry for configuration.yaml:

nzbget:
  host: 192.168.1.1
  ssl: false  

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.
@project-bot project-bot bot added this to Needs review in Dev Sep 6, 2019
@chriscla chriscla changed the title Changing nzbget component to support multiple platforms Re-factoring nzbget component to support future platform changes. Sep 6, 2019
Copy link
Member

left a comment

We shouldn't make any changes to this integration until a pypi library is used for the api interface.

homeassistant/components/nzbget/__init__.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Sep 6, 2019
Copy link
Member

left a comment

Nice!

homeassistant/components/nzbget/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/nzbget/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/nzbget/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/nzbget/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/nzbget/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

left a comment

Great! Just a small comment.

homeassistant/components/nzbget/__init__.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare changed the title Re-factoring nzbget component to support future platform changes. Refactor nzbget to support future platform changes Sep 12, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Please solve the merge conflict.

@chriscla chriscla force-pushed the chriscla:nzbget-platform branch from d68f2f6 to 468d5a4 Sep 14, 2019
@chriscla chriscla force-pushed the chriscla:nzbget-platform branch from 468d5a4 to 520107d Sep 14, 2019
@chriscla

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

@MartinHjelmare -- thank you for all the review comments and support :) It's really appreciated as someone new here.

I'm making the config change so that I can add more to the component (e.g. stop nzbget service, event when a download completes, etc...). Right now though, this change just has the sensor platform. So it's a breaking change without additional functionality. Should I go forward with this change or should I re-factor it so it's non-breaking and then do the break when I add more features? I would prefer to go forward with this and just add the new functionality asap.

Also, the CI (Overview Validate) keeps failing asking me to run the requirements script. I've done that on an updated rep, so I'm not sure what's going on.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

I think we can move forward with this PR as a breaking change.

Please run python3 -m script.hassfest to update codeowners file.

Copy link
Member

left a comment

Great!

Dev automation moved this from Review in progress to Reviewer approved Sep 15, 2019
@MartinHjelmare MartinHjelmare merged commit 57833f5 into home-assistant:dev Sep 15, 2019
11 checks passed
11 checks passed
CI Build #20190914.68 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing bca7363...6486708
Details
codecov/project 93.98% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 15, 2019
KJonline added a commit to KJonline/home-assistant that referenced this pull request Sep 15, 2019
* dev: (38 commits)
  Fix Environment Canada weather forecast, retain icon_code sensor (home-assistant#26646)
  Rename MockToggleDevice to MockToggleEntity (home-assistant#26644)
  Refactor nzbget to support future platform changes (home-assistant#26462)
  deCONZ - Remove mechanisms to import a configuration from configuration.yaml (home-assistant#26648)
  deCONZ - battery sensor instead of battery attribute (home-assistant#26591)
  Add built in weather to Homematic IP Cloud (home-assistant#26642)
  Move deCONZ services to their own file (home-assistant#26645)
  Add group attribute to Homematic IP Cloud (home-assistant#26618)
  Add iaqualink binary sensor and unique_id (home-assistant#26616)
  zha ZCL color loop effect (home-assistant#26549)
  [ci skip] Translation update
  deCONZ -  create deconz_events through sensor platform (home-assistant#26592)
  Update azure-pipelines-wheels.yml for Azure Pipelines
  Update azure-pipelines-wheels.yml
  Refactor Bluetooth Tracker to async (home-assistant#26614)
  Fix Typo (home-assistant#26612)
  [ci skip] Translation update
  Disable Watson TTS Telemetry (home-assistant#26253)
  Improve bluetooth tracker device code (home-assistant#26067)
  Bump zigpy-zigate to 0.3.1 (home-assistant#26600)
  ...
@lock lock bot locked and limited conversation to collaborators Sep 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
3 participants
You can’t perform that action at this time.