Improve error message for assert_eq/3

[xmbhasin]

Closes #505

Improves the error message for assert_eq/3 using consistent units for the input comparands (quantities). Before the change, the error message may have caused confusion as different units are used for the comparands.

Converting everything to the units of epsilon is intuitive and allows a user to see the difference in the error message in different units easily by changing the unit of epsilon without having to mess with the lhs or rhs expressions.

With this change, assert_eq/3 will convert comparands to the units of epsilon before comparison and show original units where possible in parentheses.

Example error message after this change:

>>> assert_eq(2m, 201cm, 0.1cm to mm)
error: Assertion failed
  ┌─ <input:1>:1:15
  │
1 │ assert_eq(2m, 201cm, 0.1cm to mm)
  │           --  ^^^^^ 2010.0 mm (201 cm)
  │           │
  │           2000 mm (2 m)
  │
  = Assertion failed because the following two quantities differ by 10.0 mm, which is more than or equal to 1 mm:
      2000 mm (2 m)
      2010.0 mm (201 cm)

Example error message before this change:

>>> assert_eq(2m, 201cm, 0.1cm to mm)
error: Assertion failed
  ┌─ <input:1>:1:15
  │
1 │ assert_eq(2m, 201cm, 0.1cm to mm)
  │           --  ^^^^^ 201 cm
  │           │
  │           2 m
  │
  = Assertion failed because the following two quantities differ by more than 1 mm:
      2 m
      201 cm

[xmbhasin]

One concern I had was that this could be a breaking change in some edge cases where converting everything to the epsilon units instead of the lhs units could result in a different result from assert_eq/3. I'm not sure if this is possible.

[sharkdp]

One concern I had was that this could be a breaking change in some edge cases where converting everything to the epsilon units instead of the lhs units could result in a different result from assert_eq/3. I'm not sure if this is possible.

That could very well be. But I think that should be errors on the order of the floating point precision. I wouldn't worry about it too much. It should only happen if someone has set their threshold to more or less the exact boundary value. Which is never a good idea with floating point math.

But thanks for raising the concern. In general, I'm still quite liberal with breaking changes in Numbat. The language is still very young and I'd rather get the defaults right. Also, I don't think there are any larger existing "code bases" in Numbat — yet! :smile:. Except for the ones in this repo, of course. But those are covered by tests.

[sharkdp]

I think it would be great if we could also add a few (integration) tests to numbat/tests/interpreter.rs. You can use insta for this. Basically, you add a new test like

#[test]
fn assert_eq_3() {
    insta::assert_snapshot!(fail("assert_eq(2m, 201cm, 0.1cm)"), @"");
}

and then run cargo insta test --review which should open an interactive prompt to accept the current error message and fill it in automatically.

And then we could go through a few different cases by adding more insta::assert_snapshot!(…) calls.

[xmbhasin]

Would you prefer a single squashed commit or granular commits?

[sharkdp]

Would you prefer a single squashed commit or granular commits?

Both are fine. While it's still in review, I definitely prefer granular commits. And I usually just merge/rebase as is, without squashing. Thank you for asking!

[xmbhasin]

I wanted to move interpreter.rs into a module folder for better organization (see c7fde27). I can revert that commit if you think it would break or affect other open work.