Codecov Report

Patch coverage: 80.86% and project coverage change: +2.26 🎉

Comparison is base (8a7f528) 88.67% compared to head (510d90d) 90.94%.

@@            Coverage Diff             @@
##             main    #1269      +/-   ##
==========================================
+ Coverage   88.67%   90.94%   +2.26%     
==========================================
  Files         136      158      +22     
  Lines        4187     5058     +871     
==========================================
+ Hits         3713     4600     +887     
+ Misses        474      458      -16     

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@marandaneto I randomly generated numbers to see if the formatting is the same. I have encountered an oddity from number formatter, where the 16th place after the decimal is rounded up/down. See below for a comparison. To me it looks that NumberFormat is doing the wring thing, as it is changing the input where it's not necessary. Sometimes it's rounding values >= 5 up, and sometimes down, which might come down to double imprecision in the algorithms.

Opted to vendor all the needed files with just minimal changes in the imports and folder structure. It's a lot of code just to get the exact same result as before on the 16th place after the comma. I'm not sure if this is worth it TBH.

Opted to vendor all the needed files with just minimal changes in the imports and folder structure. It's a lot of code just to get the exact same result as before on the 16th place after the comma. I'm not sure if this is worth it TBH.

I get the trade off and is indeed a lot of code, but if using the previous implementation, we need to be sure that we are not running into rounding issues, the output should be the same as the intl parse, can we do that?

@marandaneto Only thing we could try is to generate a larger number of random sample values in the tests, including intl in the test target and compare the outputs, using a range that we ate comfortable with as rounding error. 🤔

@marandaneto Only thing we could try is to generate a larger number of random sample values in the tests, including intl in the test target and compare the outputs, using a range that we ate comfortable with as rounding error. 🤔

mmm, the question is if we can avoid the routing error, unless intl does have the very same problem?

@denrase

Expected: '0.1919191919191919'
Actual: '0.191919191919192'
Which: is different.
Expected: ... 191919191919
Actual: ... 19191919192
^
Differ at offset 16

@denrase is this fixed? I saw some commits but not sure.

@marandaneto I have some weird behaviour. Since the code is from intl, i included it again (in tests only) to see if it also behaves differently on ubuntu and windows. CI was green for all tests then. So either the test with the rounding error is flaky or something else is going on.

@marandaneto I was mistaken, the test with intl failed with the same result. So the behaviour is the same as before and also with the vendored code. There are slightly different outcomes on different platforms. The result below is from using intl:

@marandaneto I was mistaken, the test with intl failed with the same result. So the behaviour is the same as before and also with the vendored code. There are slightly different outcomes on different platforms. The result below is from using intl:

That's fine then, its similar to how it was before, let's compare the output of the intl with our code result instead of hardcoded values, this should fix the test because they should have the same result.

@marandaneto I'm not sure why this would fail here. AFAIK the code changes are not related to this test.

@marandaneto I'm not sure why this would fail here. AFAIK the code changes are not related to this test.

I already merged main into this branch, this is due to a new dio version, I already merged a fix for that.