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.

Sign up for GitHub

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

Jump to bottom

Add Samsung TV config flow #28306

Merged
merged 79 commits into from
Jan 10, 2020
Merged

Add Samsung TV config flow #28306

merged 79 commits into from
Jan 10, 2020

Conversation

escoand
Copy link
Contributor

@escoand escoand commented Oct 28, 2019 •
edited by MartinHjelmare
Loading

Breaking Change:

  • Changed config from integration media_player with platform samsungtv to integration samsungtv.
  • Config options timeout removed and the default used instead.
  • Config options mac and broadcast_address removed and turn_on_action added, which is a more general and flexible approach. See the docs for more info.

Description:

Add config flow for Samsung TV integration. (next step to split up #26865)

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

samsungtv:
  - host: 192.168.12.34

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@escoand
Copy link
Contributor Author

escoand commented Oct 28, 2019

@tulindo For your information.

@escoand escoand changed the title Add Samsung TV config flow WIP: Add Samsung TV config flow Oct 28, 2019
@escoand escoand changed the title WIP: Add Samsung TV config flow Add Samsung TV config flow Oct 28, 2019
@tulindo
Copy link
Contributor

tulindo commented Oct 28, 2019

Excellent so you choosed to go for the common code.
Simple yet effective

@escoand escoand force-pushed the samsungtv_configflow branch from ad897a8 to 795962b Compare November 1, 2019 16:09
@escoand escoand force-pushed the samsungtv_configflow branch from 795962b to f1f2753 Compare November 3, 2019 12:51
@escoand
Copy link
Contributor Author

escoand commented Nov 3, 2019

@MartinHjelmare Don't know if you're the right one, but for me the tests seem ok. Two other tests from a former PR are failing with py36 because of too high log level. Should I fix this here?

@MartinHjelmare
Copy link
Member

Please fix the existing flaky tests in a separate PR so we can merge that asap. Thanks!

This was referenced Nov 3, 2019
Merged
Merged
@escoand escoand force-pushed the samsungtv_configflow branch from f1f2753 to b75d727 Compare November 4, 2019 05:45
@escoand
Copy link
Contributor Author

escoand commented Nov 4, 2019

@MartinHjelmare It's working now.

MartinHjelmare
MartinHjelmare requested changes Nov 4, 2019
View reviewed changes
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The platform setup should use async_setup_entry as entry point in all cases. We need to consolidate the old setup_platform into the new async_setup_entry.

Please move the config validation schema to the component and make it a CONFIG_SCHEMA instead. Import the config yaml to a config flow.

I haven't looked at the tests yet.

homeassistant/components/samsungtv/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/samsungtv/__init__.py Show resolved Hide resolved
@escoand escoand force-pushed the samsungtv_configflow branch from ac536b7 to df73e80 Compare November 6, 2019 22:02
@escoand
Copy link
Contributor Author

escoand commented Nov 13, 2019

@MartinHjelmare Is it correct to remove setup_platform now?

@escoand escoand force-pushed the samsungtv_configflow branch from 1479e9d to 955311c Compare January 9, 2020 13:43
@MartinHjelmare
Copy link
Member

Let me know when you're happy with testing.

@escoand
Copy link
Contributor Author

escoand commented Jan 9, 2020

Can you explain why this is failing now? https://dev.azure.com/home-assistant/Home%20Assistant/_build/results?buildId=22596&view=logs&j=67510a0b-4400-5e74-cb95-9572caf5ca54&t=f8fe93fb-5ebb-59cc-455e-c0f2d9ad3a42&l=240

@escoand
Copy link
Contributor Author

escoand commented Jan 9, 2020

OK, I think I'll postpone this topic. I'm fine now to merge this finally.

MartinHjelmare
MartinHjelmare approved these changes Jan 10, 2020
View reviewed changes
Copy link
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.

Great job! 🎉

@MartinHjelmare MartinHjelmare merged commit ef05aa2 into home-assistant:dev Jan 10, 2020
@escoand
Copy link
Contributor Author

escoand commented Jan 10, 2020 •
edited
Loading

Same to you. First I was a bit frustrated that with every commit you find another line to complain about. But after trying to review the code of somebody else I have to say this is really hard work. Thanks for your help.

@escoand escoand deleted the samsungtv_configflow branch January 10, 2020 15:59
@lock lock bot locked and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@balloob balloob balloob left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees
No one assigned
Labels
breaking-change by-code-owner cla-signed Hacktoberfest integration: samsungtv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@escoand @tulindo @MartinHjelmare @balloob @frenck @homeassistant