core

core copied to clipboard

Proposed change

SSIA

As requested by @MartinHjelmare in https://github.com/home-assistant/core/pull/78873#discussion_r976310814

Type of change

• [ ] Dependency upgrade
• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New integration (thank you!)
• [ ] New feature (which adds functionality to an existing integration)
• [ ] Deprecation (breaking change to happen in the future)
• [ ] Breaking change (fix/feature causing existing functionality to break)
• [x] Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• [ ] The code change is tested and works locally.
• [ ] Local tests pass. Your PR cannot be merged unless tests pass
• [ ] There is no commented out code in this PR.
• [ ] I have followed the development checklist
• [ ] The code has been formatted using Black (black --fast homeassistant tests)
• [ ] Tests have been added to verify that the new code works.

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

• [ ] Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• [ ] The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• [ ] New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• [ ] Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• [ ] No score or internal
• [ ] 🥈 Silver
• [ ] 🥇 Gold
• [ ] 🏆 Platinum

To help with the load of incoming pull requests:

• [ ] I have reviewed two other open pull requests in this repository.

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (select) you are listed as a code owner for? Thanks! _{^{(message by CodeOwnersMention)}}

I'm kinda lost on why we do this? (Motivation? SSIA doesn't help in this case). As in, EntityDescription are pretty much limited to their basic, descriptive, properties, not to their options/data.

Adding this level of things to the entity descriptions by default, opens up a can of worms we originally avoided. For example, effects in lights, HVAC modes and many others.

It could be fine to implement those as well, but I think it should be consistent if we do that (and not just one off).

I'm kinda lost on why we do this? (Motivation? SSIA doesn't help in this case).

I have adjusted the PR description. It originated from https://github.com/home-assistant/core/pull/78873#discussion_r976310814

We already use the descriptions for many different kind of attributes in the implementing integrations. I don't think the can of worms is closed.

We already use the descriptions for many different kind of attributes in the implementing integrations.

Yeah sure, they can extend it they way they like.

I was surprised that options was not part of the standard SelectEntityDescription and I thought it was missed in the initial implementation. Then I bumped into the issue that "we don't want a default" so I backed out. Then I received the request from @MartinHjelmare so I worked on it again and found a (maybe not pretty) solution.

Looking at the SelectEntityDescription in the core, it looks like more components would also benefit:

• kostal_plenticore, overkiz, renault and xiaomi_miio use a custom options property that it updated in this PR
• goodwe, lifx and senseme use a local constant which could be moved to the SelectEntityDescription
• litterrobot, roku, sensibo, tuya and unifiprotect use a dynamic list, and would not be impacted by this PR
• plugwise uses options in a weird way

So 7 out of 13 components would directly benefit from this, if it is accepted.

If it is not accepted, then I am happy to close (and maybe adjust the seven components to use the same code style).

@epenet for plugwise, I would suggest changing current_option: str to current_option_key: str as well. And the occurrences further down as well. current_option is used as a key, like the updated option_key.

In plugwise number.py the use of the addition _key was already implemented. Select.py was on the todo-list, but since you're already updating things, let's update all :)

@epenet for plugwise, I would suggest changing current_option: str to current_option_key: str as well. And the occurrences further down as well. current_option is used as a key, like the updated option_key.

In plugwise number.py the use of the addition _key was already implemented. Select.py was on the todo-list, but since you're already updating things, let's update all :)

I am not sure if this PR will be accepted or not. I think plugwise should be adjusted regardless to avoid confusion and I have opened the corresponding PR: https://github.com/home-assistant/core/pull/78935

There was a conflict... solved now.

👍 Will be getting back to this after the beta stuff is out.

rainmachine has crept in - I'll have to look at that tomorrow

rainmachine has crept in - I'll have to look at that tomorrow

rainmachine will be solved in #78882

Rebased over https://github.com/home-assistant/core/pull/79249

Rebased over latest dev to ensure no new components have crept in.

Some typing comments above (oh, after submitting, they are below 🙈 ).

I think if we go this route, of moving every possible attribute into the entity descriptions, we need to be consistent.

So this would be needed to be applied on all our entity platforms. For this PR that means current_option needs to be added to the entity description as well.

I think it wasn't about moving "all possible attributes" into the entity descriptions. It was more about "static attributes". Items that are set during initialisation, and not adjusted afterwards.

This is similar to what is done on the NumberEntityDescription: min value, max value and step can be set on the entity description, but the current value cannot. On SensorEntityDescription, state_class and native_unit_of_measurement can be set on the entity description, but native_value cannot.

That is the reason why I didn't implement current_option which gets adjusted throughout the life of the entity.

I have rebased over dev to pick-up lametric which also benefits from this change.

That is the reason why I didn't implement current_option which gets adjusted throughout the life of the entity.

Sure, but it can provide a default. When setting the entity description options, it still leaves room for adjusting it during the lifetime, for example, by setting self._attr_options during an update.

That is the reason why I didn't implement current_option which gets adjusted throughout the life of the entity.

Sure, but it can provide a default. When setting the entity description options, it still leaves room for adjusting it during the lifetime, for example, by setting self._attr_options during an update.

I have added it because it was requested, but I couldn't find a use case for it in the core.

I've seen you've applied current_option already, but I would love to hear the opinion of @MartinHjelmare on this as well.

I have added it because it was requested, but I couldn't find a use case for it in the core.

Well, that might be a sign not to do it 🤷 That said, select isn't a widely used platform. The default state of, e.g., switch or binary_sensors are more common maybe.

I think it's good to be consistent and have attributes for all properties in the descriptions but if we don't see a use case in the current code we could wait with adding it.

I think it's good to be consistent and have attributes for all properties in the descriptions but if we don't see a use case in the current code we could wait with adding it.

I agree that all properties in the description should have corresponding attributes, and I think that's already the case. Conversely however, I think only static attributes should have corresponding properties in the description.

Conversely however, I think only static attributes should have corresponding properties in the description.

In that case, this PR is invalid, as options doesn't have to be static...

Point taken. I should have phrased it as static or usually unchanged after initialisation

I have added a breaking change section (for custom component developers) and a blog post PR. Once this is merged I will check again all the HACS components.

Thanks, @epenet 👍