dividab / uom

Extensible unit of measure conversion with type safety for typescript
MIT License
18 stars 1 forks source link

Function Format.getUnitsForQuantity should not assume native units #41

Closed johankristiansson closed 4 years ago

johankristiansson commented 4 years ago

If you create your own unit of an existing quantity and a format for that unit you cannot use Format.getUnitsForQuantity since it assumes that you are using the native units.

Example:

const uom = require("uom");

const JohanPower = uom.Unit.timesNumber("JohanPower", 10000, uom.Units.Watt);
const JohanPowerFormat = uom.UnitFormat.createUnitFormat("JP", 0);

const myCustomUnits = {
  Watt: uom.Units.Watt,
  KiloWatt: uom.Units.KiloWatt,
  JohanPower: JohanPower
};

const myCustomUnitFormats = {
  Watt: uom.UnitsFormat.Meter,
  KiloWatt: uom.UnitsFormat.KiloWatt,
  JohanPower: JohanPowerFormat
};

const powerUnits = uom.Format.getUnitsForQuantity("Power", myCustomUnitFormats); // Crash

Instead it should accept a paramter of units, which could be defaulted to the native ones.

jonaskello commented 4 years ago

This is the docs for this function. I guess the same is true for Format.getUnitFormat(unit) then?

jonaskello commented 4 years ago

The docs seem out of sync with the impl. It already takes two params.

jonaskello commented 4 years ago

Ah, I see now there is an internal function called getUnitsPerQuantity here which is what this issue is about. I changed the title since I thought it was the public function, sorry about that :-).

jonaskello commented 4 years ago

@johankristiansson So this is what you refer to as "native units"? It is used here. You want to remove that and add it as a default param here?

johankristiansson commented 4 years ago

@jonaskello Almost correct. I want to remove it here, add a parameter here and add a default parameter here.

jonaskello commented 4 years ago

Ok that makes sense to me. I think you can go ahead with a PR for that if you want. I would be great if you could reconcile the docs at the same time.