joshwlewis / unitwise

Physical quantity and units of measure conversion and math for Ruby
unitwise.org
MIT License
281 stars 32 forks source link

Cannot register unit #51

Open pinkynrg opened 7 years ago

pinkynrg commented 7 years ago

I'm trying to register a unit called 'mma'.

Unitwise.register(
  names: "mma",
  symbol: "mma",
  primary_code: "mma",
  secondary_code: "MMA",
  scale: {
    value: 12.0,
    unit_code: 'm'
  },
  property: '...'
)

Once i do:

 Unitwise::Atom.find("mma")

I get: => #<Unitwise::Atom names=["xxx"], primary_code="mma", secondary_code="MMA", symbol="XXX", scale=#<Unitwise::Scale value=1 unit=m>, classification=nil, property="xxx", metric=false, special=false, arbitrary=false, dim="L">

but when I do this:

Unitwise(1,"mma")

I get: Unitwise::ExpressionError: Could not evaluate 'mma'.

Same thing by using the secondary_code: Unitwise::ExpressionError: Could not evaluate 'MMA'.

pinkynrg commented 7 years ago

Debugging I get this error: *** Parslet::ParseFailed Exception: Failed to match sequence (left:(GROUP / TERM)? (OPERATOR right:EXPRESSION)?) at line 1 char 3.

pinkynrg commented 7 years ago

I have reason to believe this is the rule giving issues:

rule (:term) do
  ((factor >> simpleton | simpleton | factor) >>
  exponent.maybe >> annotation.maybe).as(:term)
end

This is proven by the fact that if I change the parslet root to be simpleton mma is not recognized - with also a few more testing match, like m (meter) or mm (millimeter).

pinkynrg commented 7 years ago

I think it might be an issue with parslet https://github.com/kschiess/parslet/issues/181

pinkynrg commented 7 years ago

This is was I found:

Consider that the following rule is not a leaf of the parsing tree so if I pick wrong i'm done, considered the OR "|" sign: for details kschiess/parslet#181):

 rule (:simpleton) do
   atom | prefix >> metric_atom
 end

atom can match any measurement_unit (atoms) - OR - a prefix and any metric measurement unit.

atoms right now are:

prefix are:

This is the outcome:

mma: is an atom, match ok m: is an atom, match ok mm: is a prefix + a metric atom, doesn't match

this is because the rule matches the first "m" of "mm" as it was an atom (meter), not a prefix (milli) and since it cannot come back it throw me exception later on.

I tried also to flip the 2 parts of the rule as follows:

 rule (:simpleton) do
   prefix >> metric_atom | atom
 end

it this case:

m: since it has no prefix, go to second part of the rule and matches mm: it has prefix and it matches in the first part of the rule mma: match as it was "mm" and then it fails

pinkynrg commented 7 years ago

Solved by doing the following:

rule (:simpleton) do
  prefix.as(:prefix) >> metric_atom.as(:atom) >> simpleton_stop | atom.as(:atom) >> simpleton_stop
end

rule (:simpleton_stop) do
  match['(\.|\/|\-|\d)'].present? | any.absent?
end

Let me know what you think!

joshwlewis commented 7 years ago

Hmm, I see. Thanks for investigating. This sounds like the same issue described in #26.

Do all the tests pass with your proposed solution? My guess is that the simpleton_stop rule is consuming characters we don't want to consume (like dots, slashes, or exponents).

pinkynrg commented 7 years ago

The 2 methods absent and present are not consuming.

Sent from my Motorola XT1585 using FastHub

joshwlewis commented 7 years ago

Ok, cool. I think this would be good to bring in. Can you open a PR with these changes, but make sure that the simpleton_stop rule includes all the characters listed as exceptions in section 2.1 here (e.g. brackets, "+", "=")?

pinkynrg commented 7 years ago

Sure absolutely. I'm glad you find the fix worth it.

Sent from my Motorola XT1585 using FastHub