Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in .to('best', 'imperial') #709

Closed
kai-dorschner-twinsity opened this issue Nov 11, 2024 · 2 comments · Fixed by #710
Closed

Bug in .to('best', 'imperial') #709

kai-dorschner-twinsity opened this issue Nov 11, 2024 · 2 comments · Fixed by #710

Comments

@kai-dorschner-twinsity
Copy link

kai-dorschner-twinsity commented Nov 11, 2024

I wrote a test to verify it converts back and forth to the best fitting unit (essentially an identity conversion):

  it('checks out', () => {
    expect(convert(convert(1, 'sq mi').to('m2'), 'm2').to('sq mi')).toBe(1); // ✔️ explicit conversion works as expected
    expect(convert(convert(1, 'sq mi').to('m2'), 'm2').to('best', 'imperial')).toMatchObject({
      quantity: 1,
      unit: 'sq mi',
    }); // ❌ implicit conversion doesn't work, see screenshot
  });

Result:
image

I expect it to convert to the next highest unit when its value is >= 1 but it stays at acres (indefinitely). Even when going up to 10 or 100 square miles it is not working, see:
image

I think the same problem applies between square feet <-> acres. If you're interested I can produce a test for that as well.

For metric system everything works as expected.

@jonahsnider
Copy link
Owner

This issue doesn't have to do with converting back and forth. It's just that the array of imperial units to use for area lists acres as being larger than square miles, hence 640 ac being returned instead of 1 sq mi.

@kai-dorschner-twinsity
Copy link
Author

kai-dorschner-twinsity commented Nov 12, 2024

But... 1 acre is not larger than 1 square mile, is it?
Edit: Ah sorry, I didn't see you actually fixed it. Great news :) Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants