Feat: toBest function

[Mundi93]

Hello everyone! This pull request will try to address this thread where this feature has already been discussed extensively. The main purpose is to determine the best unit of measurement by returning a unit.

Waiting for any changes and/or adjustments!

[josdejong]

Thanks Jacopo, it looks good at first sight. I'll review your PR next week.

[josdejong]

I was looking a bit deeper into why the documentation tests are not working. Turning them off may just mask an underlying issue.

Now, I see that the issue is because the toBest method alters the value of the unit and the selected prefix, I think in the function denormalizeBest().

The idea of the prefixes is that they are readonly: you can select one, but not alter its value. For example k (or kilo is defined as 1000 meters. It has a scientifically defined value of 1000. Hence the prefix { name: 'k', value: 1000, scientific: true }. The method toBest however creates a non-existing kilo prefix with a value 1, like { name: 'k', value: 1, scientific: true }. That is wrong. It should not alter prefixes nor the unit value, it only has to select a prefix and set property .fixPrefix = true.

const unit1 = math.unit(5000, 'm')
const unit2 = unit1.to('km')
const unit3 = unit1.toBest()

print('unit1', unit1)
// unit1 {
//   string: '5 km',
//   fixPrefix: false,
//   value: 5000,
//   prefix: { name: '', value: 1, scientific: true }
// }

print('unit2', unit2)
// unit2 {
//   string: '5 km',
//   fixPrefix: true,
//   value: 5000,
//   prefix: { name: 'k', value: 1000, scientific: true }
// }

print('unit3', unit3)
// unit3 {
//   string: '5 km',
//   fixPrefix: true,
//   value: 5,
//   prefix: { name: 'k', value: 1, scientific: true }
// }
// This is not correct: value and prefix should be the same as unit2

function print(name, unit) {
  console.log(name, {
    string: unit.toString(),
    fixPrefix: unit.fixPrefix,
    value: unit.value,
    prefix: unit.units[0].prefix
  })
}

Can you look into this issue? After this is fixed, there should be no need to exclude toBest from in docs.test.js.

[josdejong]

@HeavyRainLQ I see your new commit addressing this last issue. Is it ready for review again?

[HeavyRainLQ]

Yes, I followed your suggestion, and the doc test is now passing. I had to adjust some of the toBest tests as well. It's ready for review again.

[josdejong]

Thanks a lot guys for all the updates! It has turned into a neat and solid new feature 👌. Merging now.

[josdejong]

Published now in v14.6.0. Thanks again!