Skip to content

Add ability to force number selector as an integer #130104

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
Closed
wants to merge 12 commits into from
Closed

Add ability to force number selector as an integer #130104

wants to merge 12 commits into from

Conversation

epenet
Copy link
Contributor

@epenet epenet commented Nov 8, 2024 •
edited
Loading

Breaking change

Integrations using an explicit step with an integer value will now have the value validated.

If this behavior is not desired, please adjust the step to a float (1 => 1.0)

Proposed change

As I was reviewing #130084, I realised that there was no easy way to force a NumberSelector to return an integer, and developers had to rely on vol.All(NumberSelector(...), vol.Coerce(int))

The default behavior for NumberSelector has been updated, such that setting the step as an integer will now cause validation of the value, to ensure it is an integer

To improve backwards compatibility, the default step value has been adjusted from 1 (integer) to 1.0 (float)

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)
  • Code quality improvements to existing code or addition of tests

Additional information

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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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.

To help with the load of incoming pull requests:

@home-assistant
Copy link

home-assistant bot commented Nov 8, 2024

Hey there @yuxincs, mind taking a look at this pull request as it has been labeled with an integration (apcupsd) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of apcupsd can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign apcupsd Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@home-assistant
Copy link

home-assistant bot commented Nov 8, 2024

Hey there @autinerd, mind taking a look at this pull request as it has been labeled with an integration (enigma2) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of enigma2 can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign enigma2 Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@home-assistant
Copy link

home-assistant bot commented Nov 8, 2024

Hey there @autinerd, mind taking a look at this pull request as it has been labeled with an integration (webmin) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of webmin can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign webmin Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@home-assistant
Copy link

home-assistant bot commented Nov 8, 2024

Hey there @MartinHjelmare, @functionpointer, mind taking a look at this pull request as it has been labeled with an integration (mysensors) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of mysensors can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign mysensors Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@MartinHjelmare MartinHjelmare marked this pull request as draft November 27, 2024 11:08
@MartinHjelmare MartinHjelmare added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Nov 27, 2024
@MartinHjelmare
Copy link
Member

I think this should be further discussed before we add it if at all.

@MartinHjelmare MartinHjelmare removed this from the 2024.12.0b0 milestone Nov 27, 2024
@github-actions GitHub Actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Jan 26, 2025
@github-actions github-actions bot removed the stale label Jan 26, 2025
@github-actions GitHub Actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Mar 27, 2025
@MartinHjelmare
Copy link
Member

Can we close here? I've heard no motivation why this is better than composing validators.

@github-actions github-actions bot removed the stale label Mar 27, 2025
@epenet
Copy link
Contributor Author

epenet commented Mar 28, 2025

Can we close here? I've heard no motivation why this is better than composing validators.

The biggest challenge today is that there is no way of telling the frontend to reject a decimal number.

There are a few comments above indicating that developpers would find it useful, and it would make it easier to prevent mistakes both on the developper side and on the user side.

However I don't have the energy to fight for this so...

@MartinHjelmare
Copy link
Member

If we want to extend the number selector to validate a specific type of number we should add an option for that in the number selector config. We shouldn't reuse the step option for a different purpose.

I'll close here since this isn't the right approach.

@epenet
Copy link
Contributor Author

epenet commented Mar 28, 2025

If we want to extend the number selector to validate a specific type of number we should add an option for that in the number selector config. We shouldn't reuse the step option for a different purpose.

I'll close here since this isn't the right approach.

That was what we started with: 93bcbc8

@MartinHjelmare
Copy link
Member

Then we need an architecture discussion.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@elupus elupus elupus approved these changes

@jbouwh jbouwh jbouwh approved these changes

@functionpointer functionpointer functionpointer approved these changes

@gjohansson-ST gjohansson-ST gjohansson-ST approved these changes

@MartinHjelmare MartinHjelmare Awaiting requested review from MartinHjelmare MartinHjelmare is a code owner

@emontnemery emontnemery Awaiting requested review from emontnemery emontnemery is a code owner

@bdraco bdraco Awaiting requested review from bdraco bdraco is a code owner

@farmio farmio Awaiting requested review from farmio farmio is a code owner

@marvin-w marvin-w Awaiting requested review from marvin-w marvin-w is a code owner

@frenck frenck Awaiting requested review from frenck

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@bdraco bdraco

@marvin-w marvin-w

@MartinHjelmare MartinHjelmare

@jbouwh jbouwh

@farmio farmio

@emontnemery emontnemery

Labels
cla-signed core integration: apcupsd integration: enigma2 integration: knx integration: mqtt integration: mysensors integration: webmin new-feature Quality Scale: No score Quality Scale: platinum Quality Scale: silver second-opinion-wanted Add this label when a reviewer needs a second opinion from another member.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.