home-assistant / architecture

Repo to discuss Home Assistant architecture
313 stars 100 forks source link

Register increment and decrement services for the Number integration #484

Closed JeffLIrion closed 3 years ago

JeffLIrion commented 3 years ago

Context

The Number platform allows integrations to expose numbers, and it currently registers a set_value service and provides min, max, and step attributes.

Proposal

Given the set_value service and the three aforementioned attributes, it seems only natural to also register increment and decrement services.

https://github.com/home-assistant/core/pull/45005

Consequences

Pros

Cons

None? It seems pretty straightforward.

frenck commented 3 years ago

I wonder why? As in, the goal of the entity component is not being an input_number.

Is there a specific use case for an integration that needs it? Are multiple integrations available that could benefit from this? If so, which ones?

In general, we should only start implementing these type of features, if multiple integrations could use this. Until that point is reached, I think specific integration should implement their own logic.

JeffLIrion commented 3 years ago

I was thinking in terms of UI. I think the "box" view of an input number has buttons you can click to increment/decrement the value, and I assume that a number should have the same UI as an input number.

frenck commented 3 years ago

Yeah, but that is assuming an integration supports that. Hence the question on the common use case.

JeffLIrion commented 3 years ago

Number entities have a set_value service and min, max, and step attributes. That's all that's needed for increment and decrement operations.

If an integration 's number entity doesn't support the set_value service, then it should be a sensor, not a number.

If it doesn't have a min or a max value... I don't know, can a number not have a min or a max? That doesn't seem like a realistic scenario.

frenck commented 3 years ago

Maybe, what would be the scenario to do add it? As we currently do not have a scenario yet I believe?

JeffLIrion commented 3 years ago

My main motivation is to enable better UI.

Playing around with the number platform recently, I found that the UI presents the entity just like a sensor -- there's no way to set the value, which is the key difference between a number and a sensor.

And I think that if you could change a number's value by, say, a slider, then it also makes sense to be able to increment or decrement it by clicking a button.

My thought is that a number should have the same UI as an input_number. I was going to open an issue in the frontend repo, but I realized that the number integration lacks increment and decent services, so I went ahead and submitted a pull request to add those. My plan is to open that issue after the pull request is merged.

frenck commented 3 years ago

there's no way to set the value

The UI PR isn't merged yet...

My thought is that a number should have the same UI as an input_number.

In the original architecture issue, it was decided to limits this entity for now, until more use cases arrive. Which you still haven't provided.

We should hold back on "adding features" as much as possible for the entity components.

The UI is not a backend concern.

JeffLIrion commented 3 years ago

The UI PR isn't merged yet...

I wasn't aware that there was one. I found it: https://github.com/home-assistant/frontend/pull/7876

That's what I'm really after. Increment and decrement services aren't a big deal to me, I just thought it made sense to add them.

If the decision is that these services should not be added at this time, that's fine.

frenck commented 3 years ago

I would say; lets not add them for now. We can always add them if a need for them will arise. Right now, there should not be a need yet.

balloob commented 3 years ago

Agree with Frenck. We shouldn't add things without a use case. Closing this and can be revisited in the future. If you want to revisit, open a new issue for it and reference this one.