Skip to content

LIFX: make broadcast address configurable #2967

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
Merged
merged 1 commit into from
Jul 13, 2017
Merged

LIFX: make broadcast address configurable #2967

merged 1 commit into from
Jul 13, 2017

Conversation

amelchio
Copy link
Contributor

Description:

The LIFX broadcast address can now be set.

While at it, remove server from the configuration example because listing it implies that it is needed while in reality it rarely is. If you have more than one network interface you probably know what you are doing and can figure out how to set it.

Pull request in home-assistant (if applicable): home-assistant/core#8453

@amelchio amelchio mentioned this pull request Jul 12, 2017
Merged
2 tasks
@Landrash
Copy link
Contributor

@amelchio I would prefer you keep server and change it's description. It's always better to have all the variables documented.

@Landrash Landrash added the new-feature This PR adds documentation for a new Home Assistant feature to an existing integration label Jul 13, 2017
@amelchio
Copy link
Contributor Author

Well, it is still documented (with a changed description) but just not listed in the YAML example. It's a bit unclear to me whether you missed that or you actually do think that all variables should be in the example?

I think too many variables in the examples is not always good. I have seen several users copy/paste the example, skip the description and then ask "what should I set server to" even though the description says to omit if unsure.

How about I add a second example so we have the basic and advanced setups separated and people can copy/paste the simple one?

@Landrash
Copy link
Contributor

@amelchio That was me missing that you moved it. My mistake and I'm sorry about that 🍸

Landrash
Landrash approved these changes Jul 13, 2017
View reviewed changes
Copy link
Contributor

@Landrash Landrash left a comment

Choose a reason for hiding this comment

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

Looks good and is being merged since parent PR has been merged.

@Landrash Landrash merged commit a398736 into home-assistant:next Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Landrash Landrash Landrash approved these changes

Assignees
No one assigned
Labels
new-feature This PR adds documentation for a new Home Assistant feature to an existing integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amelchio @Landrash