Skip to content

Add pressure mmhg unit conversion#35575

Closed
Limych wants to merge 2 commits intohome-assistant:devfrom
Limych:feature/pressure_mmhg
Closed

Add pressure mmhg unit conversion#35575
Limych wants to merge 2 commits intohome-assistant:devfrom
Limych:feature/pressure_mmhg

Conversation

@Limych
Copy link
Copy Markdown
Contributor

@Limych Limych commented May 13, 2020

Proposed change

In the CIS, the usual units of pressure are mmHg. This update just adds them.

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Update is simple. There are no additional info, sorry :)

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

@probot-home-assistant probot-home-assistant Bot added bugfix core small-pr PRs with less than 30 lines. labels May 13, 2020
_LOGGER = logging.getLogger(__name__)

VALID_UNITS = [PRESSURE_PA, PRESSURE_HPA, PRESSURE_MBAR, PRESSURE_INHG, PRESSURE_PSI]
VALID_UNITS = [PRESSURE_PA, PRESSURE_HPA, PRESSURE_MBAR, PRESSURE_INHG, PRESSURE_PSI, PRESSURE_MMHG]
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare May 13, 2020

Choose a reason for hiding this comment

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

Is anything using this code, ie the new unit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, there is no code yet that uses this.
But in the forum I met a lot of requests to add these units. And I myself am more familiar with them. That's why I added them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

mmHg is usual pressure unit for CIS sitizens

Copy link
Copy Markdown
Member

@frenck frenck May 13, 2020

Choose a reason for hiding this comment

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

So if nothing is using it, why should it be added? As it sounds like this will add "dead" code to our codebase.

Copy link
Copy Markdown
Contributor Author

@Limych Limych May 13, 2020

Choose a reason for hiding this comment

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

The reason is that this (and only this) unit of pressure measurement is familiar to the CIS population (and this is more than 280 million people). More precisely -- all former regions of the USSR.
Yes, this unit is not currently used in the Home Assistant code. But in the forum, people regularly ask to add it, for example, to the weather frontend.

What is the point of making “crutches” in separate modules when you can centrally add this unit to the core and solve the problem?

Copy link
Copy Markdown
Contributor Author

@Limych Limych May 13, 2020

Choose a reason for hiding this comment

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

Just because you live in a country where other standards are generally accepted, gives no reason to ignore the standards generally accepted in countries on the other sixth of all the Earth’s land.

Yes, that integration developer could theoretically suggest this code. But for some reason he did not. Maybe because he didn’t even think that it was possible? Or maybe because the conversion of pressure units was added to the core code later than he did his component?

Adding this unit will allow this and other developers to make their work easier in the future. It will allow in the future users of Home Assistant from the CIS countries to use this system with great comfort.

I am sure that a little time will pass and this code will be claimed by developers and will cease to be dead.

Copy link
Copy Markdown
Contributor Author

@Limych Limych May 13, 2020

Choose a reason for hiding this comment

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

By the way, I also found an example when a person needs reverse conversion: https://community.home-assistant.io/t/change-the-unit-in-bloomsky-sensors/35837

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We won't accept this PR until there's an active use case in the code.

Copy link
Copy Markdown
Contributor

@asmfreak asmfreak May 14, 2020

Choose a reason for hiding this comment

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

Here is a use-case out-of-code. I, as many others, want to customize output of any weather sensor, which I'm not using ATM partially because there is no pressure unit, used in my country. I could use a template, but it would be a lot nicer to put one small setting in one sensor to make it work like all other system.
Please, put yourself in our shoes and image if your country's proffered pressure/temperature/whatever unit was not supported. Let's imagine for a second, that HA was originally developed not in the US, but somewhere in Europe with metric system. I really don't think that a US citizen coming with such a small proposal adding their unit of temperature measurement, would be rejected so harshly. There was a time, when there were no temperature units in HA, and it was corrected.
As for code usage - there are CIS weather services, such as Gismeteo and Yandex Weather, providing pressure in mmHg, not in hPa. And integrations for them could have used mmHg for their operation. Please see this integration's example code. This code could have used the proposed unit.

Copy link
Copy Markdown
Member

@frenck frenck May 14, 2020

Choose a reason for hiding this comment

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

This is about adding "dead" code to our codebase. As soon as there is a use case for it in our codebase, we are happy to accept and add it.

The integration you have linked it not the Home Assistant codebase. Feel free to open up a PR to add an integration to our codebase that add this and the new integration.

PS: Did you know most of us are not in the US? Like everybody who reviewed this PR? Thanks.

@MartinHjelmare MartinHjelmare changed the title Feature/pressure mmhg Add pressure mmhg unit conversion May 13, 2020
@MartinHjelmare
Copy link
Copy Markdown
Member

Since nothing is using the code in this PR at the moment I'll close now. Please open a new PR when there's code that uses this. Thanks for your contribution.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugfix cla-signed core small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants