Skip to content

hdmi_cec enhancement #1642

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

Merged
merged 5 commits into from
Jan 21, 2017
Merged

hdmi_cec enhancement #1642

merged 5 commits into from
Jan 21, 2017

Conversation

konikvranik
Copy link
Contributor

Description:

Pull request in home-assistant: home-assistant/core#4781

@mention-bot
Copy link

@konikvranik, thanks for your PR! By analyzing the history of the files in this pull request, we identified @happyleavesaoc, @godloth and @andrew-curtis to be potential reviewers.

@Landrash Landrash added the new-feature This PR adds documentation for a new Home Assistant feature to an existing integration label Dec 23, 2016
@Landrash
Copy link
Contributor

Review of this one will wait until component is done.
Just a quick overview of this and it looks way more complicated than before to configure. If we update the config of the component we should always try to keep it compatible with the previous version to prevent breakage. If there's breakage don't forget to document it and how to update the previous configurations to match the new requirement.

@konikvranik
Copy link
Contributor Author

@Landrash
Do you find it more complicated? I changed format of config because previous approach seems complicated to me.
But maybe there is posibility to retain both approaches. If number is on left side and string on right, we use original way and if string on left and pattern %x.%x.%x.%x we use new format.

@Landrash
Copy link
Contributor

@konikvranik Your right, either will work. If there's no reason to change I would recommend not doing it as it will break some deployments of Home Assistant.

@konikvranik
Copy link
Contributor Author

@Landrash
OK, you're right, backwards compatibility is important. Reworked to offer both approaches.

@robbiet480 robbiet480 merged commit f57aedf into home-assistant:next Jan 21, 2017
@robbiet480
Copy link
Member

Thanks! 🐬 🍪 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature This PR adds documentation for a new Home Assistant feature to an existing integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants