Skip to content

added option to use a suffix for singular forms - #28

Open
Simone-RM wants to merge 1 commit into
williamhjcho:masterfrom
Robomagister:feature/singular-suffix
Open

added option to use a suffix for singular forms#28
Simone-RM wants to merge 1 commit into
williamhjcho:masterfrom
Robomagister:feature/singular-suffix

Conversation

@Simone-RM

Copy link
Copy Markdown

Having only "key" and "key_plural" breaks some translation tools which expect "key" to have no variants and only support plural forms if they all have a suffix

@williamhjcho williamhjcho left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for the PR!

Please take a look at the comments, and if possible, please also add some tests for both the I18NextOptions and for this new behavior

Comment thread lib/src/options.dart
nestingPrefix: r'$t(',
nestingSuffix: ')',
nestingSeparator: ',',
singularSuffix: 'singular',

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

to avoid breaking localization changes, the default should still be maintained

Comment thread lib/src/options.dart
Comment on lines +145 to +152
/// Defaults to 'singular' and is used for simple pluralization rules.
///
/// For example, in english where it only has singular or plural forms:
///
/// ```
/// "friend_singular": "A friend"
/// "friend_plural": "{{count}} friends"
/// ```

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

here too, the default should be null to maintain the existing behavior

Comment thread lib/src/options.dart
Comment on lines -148 to +162
/// "friend": "A friend"
/// "friend_singular": "A friend"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✂️ keep default behavior docs

Comment on lines -17 to +25
final suffix = options.pluralSuffix ?? 'plural';
return index == 0 ? '' : '$separator$suffix';
late final String suffix;

if (index == 0) {
suffix = options.singularSuffix ?? 'singular';
} else {
suffix = options.pluralSuffix ?? 'plural';
}

return suffix.isEmpty ? '' : '$separator$suffix';

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

since the base options are merged, we could do the following here to simplify this expression:

  • the singular and plural suffixes from options could just be the values themselves (from option), and remove the fallbacks from this function as to rely only on options directly.
  • then the return should check if the suffix actually exists, or if it should be ignored

this is what I mean:

      final suffix = index == 0 ? options.singularSuffix : options.pluralSuffix;
      return suffix == null || suffix.isEmpty ? '' : '$separator$suffix';

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 this pull request may close these issues.

2 participants