Having a few issues with 5.0.0 (custom group/decimal separators)

[btrautmann]

Hi there,

Sorry for the delay in testing 5.0.0. I finally got around to it, but I am having a few unit tests fail, and it's not obvious immediately whether it's my issue or the packages. All of these tests passed (with the code in the previous issue) on the prior version.

Here's my formatting function (no longer doing a temp replacement of group/decimal separators as described in #79)

String formatted(Option<CurrencyFormat> currencyFormat) {
  // TODO: This is a temporary workaround until we introduce a better way to
  // handle currrency formatting when the currency format is not yet available.
  if (currencyFormat case Some<CurrencyFormat>(:final some)) {
    // Example patterns are provided by the YNAB API without the currency
    // symbol.
    // - Replace all digits with `#`
    // - Use 0s to denote post-decimal precision
    // - Add symbol back in at the beginning or end of the pattern, as needed.
    String createPattern() {
      // e.g 123,456.789
      final sourceFormat = some.exampleFormat;
      // e.g ###,###.###
      final hashes = sourceFormat.replaceAll(RegExp(r'\d'), '#');
      final groupIndex = hashes.indexOf(some.groupSeparator);
      final decimalIndex = some.decimalDigits > 0 ? hashes.indexOf(some.decimalSeparator) : sourceFormat.length;
      final zeros = List.generate(some.decimalDigits, (i) => '0').join();
      // e.g ###,###.000
      final groupSubstring = hashes.substring(0, groupIndex);
      final decimalSubstring = hashes.substring(groupIndex + 1, decimalIndex);
      final pattern = '$groupSubstring${some.groupSeparator}$decimalSubstring${some.decimalSeparator}$zeros';
      // e.g S###,###.000
      final withSymbol = some.shouldDisplaySymbol
          ? some.isSymbolFirst
              ? 'S$pattern'
              : '${pattern}S'
          : pattern;
      print('BRANDON: withSymbol: $withSymbol');
      return withSymbol;
    }

    String symbol() => some.shouldDisplaySymbol ? some.currencySymbol : '';

    final pattern = createPattern();
    final currency = Currency.create(
      some.isoCode,
      some.decimalDigits,
      symbol: symbol(),
      pattern: pattern,
      groupSeparator: some.groupSeparator,
      decimalSeparator: some.decimalSeparator,
    );

    final minorUnits = absMinorUnitsForDecimalDigits(some.decimalDigits);
    final money = Money.fromIntWithCurrency(
      minorUnits,
      currency,
      decimalDigits: some.decimalDigits,
    );
    return _formatWithSign(money.toString());
  } else {
    // If no currency format is available, assume USD.
    final currency = Currency.create('USD', 2);
    final money = Money.fromIntWithCurrency(absMinorUnitsForDecimalDigits(2), currency);
    return _formatWithSign(money.toString());
  }
}

Here are just a few examples of failures:

test('group separator as space', () {
  const euroCurrency = CurrencyFormat(
    isoCode: 'EUR',
    decimalDigits: 2,
    decimalSeparator: ',',
    groupSeparator: ' ',
    currencySymbol: '€',
    isSymbolFirst: false,
    shouldDisplaySymbol: true,
    exampleFormat: '123 456,78',
  );

  const amount = 1234567890;
  final formatted = amount.formatted(const Some(euroCurrency));
  expect(formatted, '1 234 567,89€');
});

fails with:

BRANDON: withSymbol: ### ###,00S
Expected: '1 234 567,89€'
  Actual: '1 23 45 67 1 23 45 67€'
   Which: is different.
          Expected: 1 234 567,89€
            Actual: 1 23 45 67 1 2 ...
                        ^
           Differ at offset 4

test('standard with zeros', () {
  const euroCurrency = CurrencyFormat(
    isoCode: 'EUR',
    decimalDigits: 2,
    decimalSeparator: ',',
    groupSeparator: '.',
    currencySymbol: '€',
    isSymbolFirst: false,
    shouldDisplaySymbol: true,
    exampleFormat: '123.456,78',
  );

  const amount = 100000000;
  final formatted = amount.formatted(const Some(euroCurrency));
  expect(formatted, '100.000,00€');
});

fails with:

BRANDON: withSymbol: ###.###,00S
Expected: '100.000,00€'
  Actual: '100000,000000€'
   Which: is different.
          Expected: 100.000,00€
            Actual: 100000,000000 ...
                       ^
           Differ at offset 3

Using the same euroCurrency from the previous test:

test('zero', () {
  const amount = 0;
  final formatted = amount.formatted(const Some(euroCurrency));
  expect(formatted, '0,00€');
});

fails with

BRANDON: withSymbol: ###.###,00S
Expected: '0,00€'
  Actual: '0,000000€'
   Which: is different.
          Expected: 0,00€
            Actual: 0,000000€
                        ^
           Differ at offset 4

test('decimal separator as hyphen', () {
  const euroCurrency = CurrencyFormat(
    isoCode: 'EUR',
    decimalDigits: 2,
    decimalSeparator: '-',
    groupSeparator: ' ',
    currencySymbol: '€',
    isSymbolFirst: false,
    shouldDisplaySymbol: true,
    exampleFormat: '123 456-78',
  );

  const amount = 1234567890;
  final formatted = amount.formatted(const Some(euroCurrency));
  expect(formatted, '1 234 567-89€');
});

fails with (note: the same failure occurs for / as group separator) :

BRANDON: withSymbol: ### ###-00S
The pattern contains an unknown character: '-'
package:money2/src/pattern_encoder.dart 206:11  PatternEncoder._getMoneyPattern
package:money2/src/pattern_encoder.dart 78:26   PatternEncoder._formatMajorPart
package:money2/src/pattern_encoder.dart 53:17   PatternEncoder.encode
package:money2/src/money.dart 483:15            Money.encodedBy
package:money2/src/money.dart 445:24            Money.toString
package:lumy/ui/currency.dart 61:34             CurrencyIntX.formatted
test/ui/currency_test.dart 214:34               main.<fn>.<fn>.<fn>

[bsutton]

whilst I think its a bug, allowing a '-' as decimal separator seems highly problematic due to confusion with a -ve value.

I'm inclined to disallow - and + as any type of separator.

[btrautmann]

whilst I think its a bug, allowing a '-' as decimal separator seems highly problematic due to confusion with a -ve value.

I'm inclined to disallow - and + as any type of separator.

That's fair, I just don't really have a choice but to support it 😅

[bsutton]

I'm going to close this as whilst it might be a bug I don't think supporting a +- as a decimal separator is worth investing time into.
If you want to submit a PR with a fix then I would be happy to accept it.

[btrautmann]

I'm going to close this as whilst it might be a bug I don't think supporting a +- as a decimal separator is worth investing time into. If you want to submit a PR with a fix then I would be happy to accept it.

Two of the failure cases had , and . or as group/decimal separators, not sure if you saw that. Those feel more like bugs that are worth addressing.

[bsutton]

If you can give me some specific unit tests (that don't use your FormatCurrency function ) then I would be happy to look at it.

I want to exclude the FormatCurrency as I don't want to be testing that as opposed to testing the core money classes.