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

Centrally define and expand units of measurement #164

Closed
frenck opened this issue Feb 28, 2019 · 13 comments · Fixed by home-assistant/core#21570 or home-assistant/core#21719
Closed

Comments

@frenck
Copy link
Member

frenck commented Feb 28, 2019

Looking at the current code base, I amazed myself with the current set of unit types defined as a constant:

Copy past from the current state of homeassistant.const:

# #### UNITS OF MEASUREMENT ####
# Temperature units
TEMP_CELSIUS = '°C'
TEMP_FAHRENHEIT = '°F'

# Length units
LENGTH_CENTIMETERS = 'cm'  # type: str
LENGTH_METERS = 'm'  # type: str
LENGTH_KILOMETERS = 'km'  # type: str

LENGTH_INCHES = 'in'  # type: str
LENGTH_FEET = 'ft'  # type: str
LENGTH_YARD = 'yd'  # type: str
LENGTH_MILES = 'mi'  # type: str

# Volume units
VOLUME_LITERS = 'L'  # type: str
VOLUME_MILLILITERS = 'mL'  # type: str

VOLUME_GALLONS = 'gal'  # type: str
VOLUME_FLUID_OUNCE = 'fl. oz.'  # type: str

# Mass units
MASS_GRAMS = 'g'  # type: str
MASS_KILOGRAMS = 'kg'  # type: str

MASS_OUNCES = 'oz'  # type: str
MASS_POUNDS = 'lb'  # type: str

# UV Index units
UNIT_UV_INDEX = 'UV index'  # type: str

This list isn't extensive and doesn't cover the measurement units used across the code base.

Proposal

Extend the above list with measurement units used across the codebase in case they are used multiple times. This prevents the units from being defined everywhere (again and again) and can potentially help to set a ground for more extensive unit conversion handling in the future (oppertunity).

@balloob
Copy link
Member

balloob commented Feb 28, 2019

I think that we might want to differentiate between standardized and non-standardized units. Otherwise this file will get HUGE, with weird ones like this:

https://github.com/home-assistant/home-assistant/blob/127c55e0c196dc6ffd321c1ad28b533f636faf79/homeassistant/components/verisure/sensor.py#L145-L148

Standardized units are ones used by entity components or (binary) sensor with device classes.

@frenck
Copy link
Member Author

frenck commented Feb 28, 2019

"in case they are used multiple times" is definitely an important part of the proposal 😉

We could stick to the ISO 80000 standard (Describing scientific and mathematical quantities and their units) as a general requirement for them being in the global scope?

https://en.wikipedia.org/wiki/ISO/IEC_80000

Nevertheless, I think a good amount PR reviewing (<- definitely not a unit of measurement) will do the trick as well.

@dgomes
Copy link

dgomes commented Mar 1, 2019

So should we start opening PR's?

I can have a go with "W" (watts) for power (I can count 39 occurrences)

@rytilahti
Copy link
Member

Just my 0.02e; would it make sense to think about re-using some already available unit lib (short googling revealed that https://pint.readthedocs.io/en/latest/ could be a candidate) for defining the types? It is not that these change so often, and having the ability to contain the type & value in one container could be leveraged (later on) on typed state attributes (instead of coding the type in the attribute name).

@OttoWinter
Copy link
Member

@rytilahti For that see #10

@rytilahti
Copy link
Member

@OttoWinter ah, well, making unit conversions simpler is would be a side-effect indeed. What I just wanted to say is that instead of maintaining a list of units inside homeassistant's code-base, using an external library would make (in my opinion) sense and lessen the maintenance burden.

In the long run this would then allow simplifying platform code by stripping the conversions away and letting the core to handle that on their behalf. I.e. with #146, the hue component would report that it's input & output types for color temperature are in mireds and that's all; the users could define whatever compatible type they want as an input and it would simply work.

@dgomes
Copy link

dgomes commented Mar 1, 2019

I agree with you @rytilahti but if we start somewhere (this case centralize all references) then we can easily go to this file and replace all components unit implementations.

@frenck
Copy link
Member Author

frenck commented Mar 2, 2019

The point @dgomes makes was part of my idea/proposal indeed. Just dropping in some library is not going to do it right now. Take control on something first, then change it.

@MatthewFlamm
Copy link
Contributor

MatthewFlamm commented Mar 4, 2019

I added pressure units in homeasssistant#2120 as part of fixing pressure units in Dark Sky. But that PR does not address other components or even other platforms within the default weather component. I think it would be worthwhile for someone to review the choices for the pressure units based on this proposal here.

@frenck frenck changed the title Proposal: Centrally define and expand units of measurement Centrally define and expand units of measurement Mar 5, 2019
@dgomes
Copy link

dgomes commented Mar 6, 2019

I think we should discuss in this issue how to handle "misbehaving" units.

Looking through the code base the worst seams to be '°C' vs 'C'. Should we promote a breaking change ? Or just leave things as they are ?

@rytilahti
Copy link
Member

If the plan is to move away from the string-based units in the near future, it would be IMO better to do the breaking change at that point to make everything consistent for good. I seriously think that this should be already discussed at this point in order to decide whether it makes sense to start mass-converting units now and then redo the same task after a while again.

After testing a pint briefly, I'm not really convinced on it as due to its dynamic nature there are no type hints for the types and thus no autocompletion in IDEs. This could maybe be alleviated by providing the typing hints ourselves, though. (personally I would like to just see hass.units module for all our unit needs instead of having a bunch of globals one needs to remember to import. This is for developer friendliness after all, hunting for consts is not very enjoyable experience and I think this is the reason why people have been defining the types themselves in the first place.

@frenck
Copy link
Member Author

frenck commented Mar 7, 2019

Looking through the code base the worst seams to be '°C' vs 'C'. Should we promote a breaking change ? Or just leave things as they are ?

One way or the other, eventually, if #10 is something we are going for, it is going to be a breaking change.

personally I would like to just see hass.units module for all our unit needs instead of having a bunch of globals one needs to remember to import.

I agree, however, I feel like moving onto globally defined constants will help to achieve that in the future. Especially, since we now have things like the above °C.

I'm fine with defining a hass.units module, nevertheless, that would hit #10 right away.

@awarecan
Copy link

awarecan commented Mar 9, 2019

Semi-related, I am thinking propose a solution to translate (no convert) unit in frontend, centrally managed unit will help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants