search

Shopify / measured

Encapsulate measurements and their units in Ruby and Ruby on Rails.
MIT License
337 stars 28 forks source link

Custom unit alias regression in 2.5.1 #127

Closed trcarden closed 4 years ago

trcarden commented 4 years ago

We recently upgraded out gems and moved from measured 2.4 to 2.5.1 and noticed that some of our custom units and aliases stopped working.

Here is a fabricated example:

In measured.rb initializer

Measured::Cardinality = Measured.build do
  unit :piece, aliases: [:pieces]
  unit :dozen, aliases: [:dz], value: "12 pieces"
end

Key issue:

Previous/Expected behavior:

Observed behavior:


Measured::UnitError: Cannot find conversion path from dozen to piece.
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/conversion_table_builder.rb:43:in `find_conversion'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/conversion_table_builder.rb:31:in `block (2 levels) in generate_table'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/conversion_table_builder.rb:30:in `each'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/conversion_table_builder.rb:30:in `block in generate_table'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/conversion_table_builder.rb:27:in `each'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/conversion_table_builder.rb:27:in `each_with_object'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/conversion_table_builder.rb:27:in `generate_table'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/conversion_table_builder.rb:13:in `to_h'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/unit_system.rb:13:in `initialize'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/unit_system_builder.rb:24:in `new'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/unit_system_builder.rb:24:in `build'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/base.rb:21:in `block in build'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/base.rb:16:in `initialize'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/base.rb:16:in `new'
/Users/trcarden/.rvm/gems/ruby-2.6.5@titan/gems/measured-2.5.1/lib/measured/base.rb:16:in `build'
***/measured.rb:6:in `<main>'```
kmcphillips commented 4 years ago

Interesting. We'll have to start by figuring out of it's the alias or the conversion table that's regressed.

kmcphillips commented 4 years ago

I did a bisect and fortunately it appears to be the most recent commit 7a4b31704dc00c35da186223ac8468bb03e344ff from #126.

I can't promise I'll fix this immediately, so in the mean time @trcarden you can downgrade to 2.5.0. There's only the one PR in the most recent release.

kmcphillips commented 4 years ago

No, actually that is not correct. The bug persists all the way back to 2.4.0. The only difference is the conversion table is eagerly calculated in 2.5.1 so @conversion_table_builder.to_h is called in initialization, not on first use, meaning you trigger the same error on use and not on load.

kmcphillips commented 4 years ago

So this is actually just a thing that's not supported. I guess it could be. But to define a unit system it expects you to define the value by unit name and not by unit alias. It's a feature we could support.

The provided example works by removing the s and just defining "12 piece".