ChHecker / unify

Format numbers, units, and ranges in Typst correctly.
MIT License
23 stars 17 forks source link

Result of `#unit("Hz")` is confusing #3

Closed YDX-2147483647 closed 1 year ago

YDX-2147483647 commented 1 year ago

Results of #qty is similar.

I am using v0.4.2 and I've verified this issue on current main (https://github.com/ChHecker/unify/commit/d2ab4c363246a84e82a6a82e842c397f25716b32).

Workaround

Don't use units-short: #unit("hertz") works as expected.

Reason

Therefore the checks pass, and we try to parse z and fail. However, we don't #panic and let it go…

Possible Fix

In #format-unit-short, remove the else if in the for loop… https://github.com/ChHecker/unify/blob/d2ab4c363246a84e82a6a82e842c397f25716b32/lib.typ#L230-L232 …and add the following immediately after the loop.

    if acc.len() > 0 {
      panic("invalid unit: " + quantity)
    }

This will make #unit("hz") into error: panicked with: "invalid unit: hz".

In the same for loop, add and unit == none in this if, because the prefix must appear before the unit: https://github.com/ChHecker/unify/blob/d2ab4c363246a84e82a6a82e842c397f25716b32/lib.typ#L223-L225

This will make #unit("Hz") into error: panicked with: "invalid unit: Hz".

[!WARNING]

This does NOT make #unit("Hz")Hz, because #format-unit-short is eager — it consumes H as Henry first.

A Bigger Refactor?

I suggest a bigger refactor of the parsing logic in #format-unit-short

  1. If quantity in units-short, unit = units-short.at(quantity), ✓.
  2. If not, divide quantitiy into two parts, if the first part is a prefix and the second is a unit, ✓.
  3. If not, try to divide at other positions.
  4. If all division fail, panic.

How do you think so? If you agree but are busy, I can try to work out a PR.