Skip to content

Add config flow to transmission#26434

Merged
MartinHjelmare merged 13 commits into
home-assistant:devfrom
engrbm87:transmission
Sep 26, 2019
Merged

Add config flow to transmission#26434
MartinHjelmare merged 13 commits into
home-assistant:devfrom
engrbm87:transmission

Conversation

@engrbm87
Copy link
Copy Markdown
Contributor

@engrbm87 engrbm87 commented Sep 4, 2019

Breaking Change:

The Transmission integration can now be configured through a config flow via Integrations in the GUI. Once configured all sensors and switches will be created and available for the user.
monitored_conditions has been removed so existing users need to update their configuration in configuration.yaml and remove monitored conditions. The existing configuration will be imported as an entry under Integrations.

Description:

In addition to the previous sensors and switches that were available under monitored_conditions a new switch to start and stop all torrents has been added.
The update Interval is now an option that can be altered under options.

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

Example entry for configuration.yaml (if applicable):

transmission:
  host: 192.168.1.1

Checklist:

  • The code change is tested and works locally.
  • 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.

@engrbm87
Copy link
Copy Markdown
Contributor Author

engrbm87 commented Sep 5, 2019

Open Issue : 26334

Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/config_flow.py Outdated
Comment thread homeassistant/components/transmission/const.py Outdated
Comment thread homeassistant/components/transmission/strings.json Outdated
Comment thread homeassistant/components/transmission/switch.py Outdated
Comment thread homeassistant/components/transmission/config_flow.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/switch.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/config_flow.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Since the library doesn't support asyncio, we have to think about sync vs async context all the time in this integration. Please take care, especially when calling api functions.

I think we have a good separation now, considering this complication. All the methods on the TransmissionClient class are async. All the methods on the TransmissionData class are sync.

Comment thread homeassistant/components/transmission/__init__.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Sep 9, 2019

Please run python3 -m script.hassfest.

Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
@engrbm87
Copy link
Copy Markdown
Contributor Author

engrbm87 commented Sep 9, 2019

Please run python3 -m script.hassfest.

There is an issue with this script and the black pre-commit (issue#26334). transmission is already in the list in this file

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Good!

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Sorry, we require tests of the config flow too. It should be included in coverage calculation.

Comment thread tests/components/transmission/test_config_flow.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread tests/components/transmission/test_config_flow.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

Please fix the CLA error.

@MartinHjelmare
Copy link
Copy Markdown
Member

File config_flows.py is not up to date. Please run python3 -m script.hassfest.

@engrbm87
Copy link
Copy Markdown
Contributor Author

File config_flows.py is not up to date. Please run python3 -m script.hassfest.

I am not able to commit after running the script. Black is adding a comma to the last line.

@MartinHjelmare
Copy link
Copy Markdown
Member

Try rebasing on latest dev branch if that problem persists. We should have fixed it in the dev branch by turning off formattting of the generated files.

@MartinHjelmare
Copy link
Copy Markdown
Member

Looks good! Please update .coveragerc and don't exclude the config flow module from coverage calculation.

@frenck
Copy link
Copy Markdown
Member

frenck commented Sep 20, 2019

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

@engrbm87
Copy link
Copy Markdown
Contributor Author

engrbm87 commented Sep 21, 2019

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

Thanks for the opportunity to contribute in HA which I really like. I have added my username as code owner.

@MartinHjelmare
Copy link
Copy Markdown
Member

Please add tests of the options flow too, to reach 100% test coverage of the config flow module.

Comment thread tests/components/transmission/test_config_flow.py Outdated
Comment thread tests/components/transmission/test_config_flow.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare 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!

@MartinHjelmare
Copy link
Copy Markdown
Member

Please add a breaking change paragraph in the PR description and update the description with the current changes in the PR. The breaking change paragraph should describe briefly what changed and what the user needs to do to cope with the breaking change.

@MartinHjelmare MartinHjelmare merged commit 82b77c2 into home-assistant:dev Sep 26, 2019
@lock lock Bot locked and limited conversation to collaborators Sep 27, 2019
@engrbm87 engrbm87 deleted the transmission branch September 16, 2022 09:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants