Skip to content

Conversation

@shihaohong
Copy link
Contributor

@shihaohong shihaohong commented Oct 30, 2020

Fixes #69345, which fails because of a change that uses raw strings as part of the code generation introduced by #69025.

This PR simply reverts the generateString logic for the gen_l10n tool, but keeps the Material/Cupertino generated localizations one intact. Since the introduced logic prefers raw strings (which the flutter/flutter linters now enforce), this is necessary for the Material and Cupertino library code generators. However, this approach fails with gen_l10n.

For example:

price: $4.45 => "r'price: $4.45'"
get String getPriceOfItem => r'price: $4.45';

^ This is okay in our cupertino and material delegate generation code because it parses raw strings with interpolated variables differently (see this example).

However, in gen_l10n, this becomes a little weird, both in the code comments:

/// Example of the code that is generated with the string above
/// is r'price: $4.45' but we really want 'price: $4.45'

and also in the delegate code:

  @override
  String price(int priceValue) {
    final intl.NumberFormat priceValueNumberFormat = intl.NumberFormat.decimalPattern(
      locale: localeName,
      
    );
    final String priceValueString = priceValueNumberFormat.format(priceValue);

    return r'$${priceValueString} SAR';
  }

instead of

  @override
  String price(int priceValue) {
    final intl.NumberFormat priceValueNumberFormat = intl.NumberFormat.decimalPattern(
      locale: localeName,
      
    );
    final String priceValueString = priceValueNumberFormat.format(priceValue);

    return '${priceValueString} SAR';
  }

Related Issues

Fixes #69345

Tests

I added the following tests:

  • Fixes the failing unit test that was turned off.
  • Added a new integration test for dollar signs in the arb file
  • Modified an existing currency integration test which would fail if a non-breaking space character wasn't used. It is too hard to decipher and not easy to create with the keyboard, so I moved it to a language (zh) that doesn't contain the character.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 30, 2020
@google-cla google-cla bot added the cla: yes label Oct 30, 2020
@shihaohong shihaohong added the a: internationalization Supporting other languages or locales. (aka i18n) label Oct 30, 2020
@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Oct 30, 2020
@shihaohong shihaohong added the c: regression It was better in the past than it is now label Oct 30, 2020
@shihaohong shihaohong changed the title [gen_l10n] Fix unintended addition of "r" character due to use of raw strings [gen_l10n] Fix unintended use of raw string in generateString Oct 30, 2020
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (but not to CI...)

@shihaohong shihaohong merged commit 4996f60 into flutter:master Oct 31, 2020
@shihaohong shihaohong deleted the failing-genl10n-test branch October 31, 2020 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: internationalization Supporting other languages or locales. (aka i18n) c: contributor-productivity Team-specific productivity, code health, technical debt. c: regression It was better in the past than it is now tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[gen_l10n] Raw strings are being incorrectly used when generating messages

2 participants