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

Support for multiple min-max sensors #12500

Closed
wants to merge 1 commit into from
Closed

Support for multiple min-max sensors #12500

wants to merge 1 commit into from

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 18, 2018

Description:

I updated the component schema, to account for multiple min_max sensors. Other changes include:

  • Changed name to friendly_name
  • Small improvements
  • Updated tests

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4700

Breaking changes

The min/max sensor now supports adding multiple sensors without the need to add a new platform entry. For information on how to update your configurations file, refer to the component page Min/Max Sensor.

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: min_max
    sensors:
      my_max_sensor:
        entity_ids:
          - sensor.kitchen_temperature
          - sensor.living_room_temperature
          - sensor.office_temperature

Checklist:

  • The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

* Breaking Change: Updated Schema to support multiple Min_Max Sensors
* Breaking Change: Changed name to friendly_name
* Small imporvements
* Updated tests
@OttoWinter
Copy link
Member

I'm sorry if this has been answered, but why do we even need this? I mean adding a new platform: entry for each sensor would make more sense to me. It's also something I find very confusing about the template platform, as there are now two ways of achieving the same thing.

Is it somehow related to performance or consistency? Because I can't think of any other reason...

@fabaff
Copy link
Member

fabaff commented Feb 18, 2018

There are two different styles available to achieve the same thing when it comes to the setup of entities. We have no guidelines on this and probably will never have. It pretty much depends on the person who create/created the integration. I prefer the multiple platform approach. Because it's easier to use for newbies, is more flexible if one want to split the configuration (aka one file for one entity vs. all min_max sensors at one place) and way less sensitive to indent.

Change the style of one integration will not make the setup or configuration of Home Assistant at lot easier.
Also, I see no real value in adding a breaking change for a style adjustment which is not significantly simpler.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 18, 2018

@OttoWinter I originally started this PR because of inconsistencies with some template platforms and the input components (these are the cases I know of).
Without knowing the definitive answer to your question, their are some good points for doing it this way:

  • async_add_entities helpers/entity_platform takes a list of new_entities as an input.
  • This option leads to better entity_ids since they are assigned by the user.
  • The configuration file in general is structured in a way that encourages keeping components of the same platform together. Everything needs to be group under their domain in the first place. You can misc components together to say create a file that contains every component for just one room.
  • The other schema, at least for me, does only make sense if you are dealing with platforms that you need just ones, like a yr weather for example.

@fabaff IMO is about consistency across different components. This PR is meant as a suggestion to increase such.

@fabaff
Copy link
Member

fabaff commented Feb 18, 2018

If you talk about consistency then it should be the other way around. The platforms/components which support the multiple entities style are in the minority (at least, last time I checked when this topic was coming up).

This option leads to better entity_ids since they are assigned by the user.

I disagree. People are less creative than we think when it comes to name things. The current way with the default name is already covering this even when there is no name: used.

The configuration file in general is structured in a way that encourages keeping components of the same platform together. Everything needs to be group under their domain in the first place. You can misc components together to say create a file that contains every component for just one room.

Not really. The structure of the configuration file is independent from the style and works with multiple <platform x>, <platform>: and list or other variations including the options for splitting the configuration file.

The other schema, at least for me, does only make sense if you are dealing with platforms that you need just ones, like a yr weather for example.

I'm sure that there are people out there who are using the same platform multiple times. They want to know how the weather is at their holiday home or in their favorite ski area and use the same service/platform.

It would be easier to move this discussion to https://github.com/home-assistant/architecture to keep this PR short.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 20, 2018

Closing it, see: home-assistant/architecture#12

@cdce8p cdce8p closed this Feb 20, 2018
@cdce8p cdce8p deleted the min-max_sensor branch February 20, 2018 22:16
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
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.

None yet

4 participants