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

Add mA to SensorDeviceClass.CURRENT units #84492

Merged
merged 4 commits into from
Dec 30, 2022

Conversation

SukramJ
Copy link
Contributor

@SukramJ SukramJ commented Dec 23, 2022

Proposed change

Add mA to SensorDeviceClass.CURRENT unit assignement.
This one was missing in #84366

Fixes: ... is using native unit of measurement 'mA' which is not a valid unit for the device class ('current') it is using; Please update your configuration if your entity is manually configured, ...

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
  • 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:

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.

To help with the load of incoming pull requests:

@home-assistant
Copy link

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

Code owner commands

Code owners of sensor can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign sensor Removes the current integration label and assignees on the issue, add the integration domain after the command.

@SukramJ SukramJ changed the title Add mA to SensorDeviceClass.CURRENT unit assignement Add mA to SensorDeviceClass.CURRENT unit assignement to fix warnings in log Dec 26, 2022
Copy link
Contributor

@epenet epenet left a comment

Choose a reason for hiding this comment

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

Please ensure that:

  • you update the NumberDeviceClass docstring accordingly
  • the documentation is updated for both number and sensor platforms
  • both the public and the developer documentation are updated.

@epenet epenet changed the title Add mA to SensorDeviceClass.CURRENT unit assignement to fix warnings in log Add mA to SensorDeviceClass.CURRENT unit assignement Dec 26, 2022
@epenet epenet changed the title Add mA to SensorDeviceClass.CURRENT unit assignement Add mA to SensorDeviceClass.CURRENT units Dec 26, 2022
@SukramJ
Copy link
Contributor Author

SukramJ commented Dec 26, 2022

  • you update the NumberDeviceClass docstring accordingly
  • the documentation is updated for both number and sensor platforms
  • both the public and the developer documentation are updated.

Done

@epenet
Copy link
Contributor

epenet commented Dec 26, 2022

NumberDeviceClass docstring hasn't yet been adjusted

@SukramJ
Copy link
Contributor Author

SukramJ commented Dec 26, 2022

Sorry, missing push.

epenet
epenet previously approved these changes Dec 26, 2022
Copy link
Contributor

@epenet epenet left a comment

Choose a reason for hiding this comment

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

LGTM

@frenck
Copy link
Member

frenck commented Dec 27, 2022

This one was not missing; it was never supported, to begin with?

The developer documentation always has been clear about that (which is evident since this PR also adjusts those).

Should this be approved architecturally first? I'm not sure why they weren't supported; there might be reasoning for that.

../Frenck

@SukramJ
Copy link
Contributor Author

SukramJ commented Dec 27, 2022

mA is already in use by wled, tuya, homematic, issy994 and custom_components.
What would be your proposal? Convert mA to A? With how many decimal places?

@frenck
Copy link
Member

frenck commented Dec 27, 2022

mA is already in use by wled, tuya, homematic, issy994 and custom_components.

Which thus is incorrect according to our current developer documentation.

What would be your proposal?

As per last comment:

Should this be approved architecturally first? I'm not sure why they weren't supported; there might be reasoning for that.

@SukramJ
Copy link
Contributor Author

SukramJ commented Dec 27, 2022

I home that discussion helps.

@epenet
Copy link
Contributor

epenet commented Dec 27, 2022

Side note, based on latest comments: Tuya currently implements a conversion function.

UnitOfMeasurement(
unit=UnitOfElectricCurrent.MILLIAMPERE,
aliases={"ma", "milliampere"},
device_classes={SensorDeviceClass.CURRENT},
conversion_unit=UnitOfElectricCurrent.AMPERE,
conversion_fn=lambda x: x / 1000,
),

homeassistant/util/unit_conversion.py Outdated Show resolved Hide resolved
homeassistant/util/unit_conversion.py Outdated Show resolved Hide resolved
homeassistant/util/unit_conversion.py Outdated Show resolved Hide resolved
homeassistant/util/unit_conversion.py Outdated Show resolved Hide resolved
@epenet
Copy link
Contributor

epenet commented Dec 29, 2022

Note: there is a conflict from recently added data_size/data_rate converters.

@SukramJ
Copy link
Contributor Author

SukramJ commented Dec 29, 2022

Please have a look at the linked frontend PR.
Question regarding the used key: Is it the UNIT_CLASS of the converter?

@epenet
Copy link
Contributor

epenet commented Dec 29, 2022

Please have a look at the linked frontend PR. Question regarding the used key: Is it the UNIT_CLASS of the converter?

Frontend is based on the device class.
I'd need to check the code to remember where UNIT_CLASS is used.

@SukramJ
Copy link
Contributor Author

SukramJ commented Dec 29, 2022

Thanks, i fixed the frontend PR.

@frenck frenck added this to the 2023.1.0 milestone Dec 30, 2022
@frenck frenck force-pushed the fix-unit-assignement branch from 648822f to d389956 Compare December 30, 2022 11:41
@frenck
Copy link
Member

frenck commented Dec 30, 2022

Rebased the PR to deal with a merge conflict

@MartinHjelmare
Copy link
Member

There's still a conflict it seems.

@frenck
Copy link
Member

frenck commented Dec 30, 2022

It is a new conflict caused by the merge of the mV PR, will pick it up.

Edit: Rebased again.

@frenck frenck force-pushed the fix-unit-assignement branch from d389956 to 55f2c76 Compare December 30, 2022 12:01
@frenck frenck merged commit 005bc89 into home-assistant:dev Dec 30, 2022
@SukramJ SukramJ deleted the fix-unit-assignement branch December 30, 2022 13:39
@frenck frenck modified the milestone: 2023.1.0 Dec 30, 2022
frenck pushed a commit that referenced this pull request Dec 30, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2022
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.

native unit of measurement 'mA' is not a valid unit for the device class ('current')
5 participants