dart-lang / i18n

A general mono-repo for Dart i18n and l10n packages.
BSD 3-Clause "New" or "Revised" License
58 stars 36 forks source link

Enums should be easier to use with the Intl.select #601

Open moodysalem opened 5 years ago

moodysalem commented 5 years ago

Issue

I have an enum

enum Thing {
 ONE, TWO
}

and I want to present some text based on the enum

thingDisplayText(Thing thing) => 
  Intl.select(thing, { Thing.ONE: 'Thing one', Thing.TWO: 'Thing two' });

It doesn't work because:

Fix

Change Intl.select to be generic, so the type of the first argument matches the key of the cases map, and move the 'other' parameter into an optional parameter instead of a key in the map

~Workaround~

thingDisplayText(Thing thing) => 
  Intl.select(thing?.toString(), { 'Thing.ONE': 'Thing one', 'Thing.TWO': 'Thing two', 'other': 'Not a thing' });
moodysalem commented 5 years ago

The workaround doesn't work because of some validation that prevents the period from being in there. Why? Invalid select keyword: 'Thing.ONE', must match '[a-zA-Z][a-zA-Z0-9_-]*'

moodysalem commented 5 years ago

This also doesn't work. Why?

thingDisplayText(Thing thing) => 
  Intl.select(thing?.toString()?.split('.')?.last, { 'ONE': 'Thing one', 'TWO': 'Thing two', 'other': 'Not a thing' });

Error (Invalid argument to plural/gender/select, must be simple variable reference)

alan-knight commented 5 years ago

Yes, this is a long-standing irritation. One issue is that it's hard to identify enums, and we need to stringify them to get them translated. The way their toString() works is very annoying.

You would have to do the splitting in a wrapper method.

I believe the ICU format for select as a string doesn't allow the periods.

The first argument doesn't have to be a string, we will apply the toString to it. So I believe that

thingDisplayText(Thing thing) => 
  Intl.select(thing, { 'Thing.ONE': 'Thing one', 'Thing.TWO': 'Thing two' });

would work except for the validation.

moodysalem commented 5 years ago

Is fixing this issue on the roadmap? Is it intentional to force the keys of the choices to be strings?

So the workaround I went with is this

thingDisplayText(Thing thing) => 
  _thingDisplayText(thing?.toString()?.split('.')?.last);

_thingDisplayText(String thing) => 
  Intl.select(thing, { 'ONE': 'Thing one', 'TWO': 'Thing two', 'other': 'Not a thing' });
alan-knight commented 5 years ago

There aren't a lot of cycles for this... but it's Friday afternoon of a long weekend, so I just did it. It's not generic, the Map is <Object, String>, which is simpler because we don't need "other" to be a special case. It's being done internally, but should get to here next week.

moodysalem commented 5 years ago

Thanks! I assume there are no issues with the toString method of the enum and the validation that prevents periods.

Also, why did you prefer using Object to generic? I would prefer type safety to 'simpler' in this case, though I understand it would require a big migration of the existing code

alan-knight commented 5 years ago

Basically I hacked it so that if the toString contains a period we take only the portion after the period. I preferred using Object because that way you can still have the required 'other' case in the map. Otherwise it would have to be moved to a separate parameter, which is a breaking change for any usage. Also, it's entirely typesafe. We use no property of the input except equality testing, which is available on Object, so anything unexpected is just 'other'. I suppose it's even theoretically possible to pass instances of multiple different things to the select, though I'd struggle to come up with a convincing example.

It's now synced into github, though not yet part of a release published to pub.

moodysalem commented 5 years ago

I see this error during message extraction when trying to use enums in this way and providing enum values for the examples:

    reason: Examples must be a const Map literal.
alan-knight commented 5 years ago

Is your examples parameter a const Map literal? Do you have an example?

moodysalem commented 5 years ago

Yes, it's like this:

thingDisplayText(Thing thing) => 
  Intl.select(thing, 
    { Thing.ONE: 'Thing one', Thing.TWO: 'Thing two', 'other': 'Not a thing' },
    examples: const {'thing': Thing.ONE});
eernstg commented 1 year ago

Just passing by and noticing a comment here:

/// It is possible to use a Dart enum as the choice and as the
/// key in cases, but note that we will process this by truncating
/// toString() of the enum and using just the name part. We will
/// do this for any class or strings that are passed, since we
/// can't actually identify if something is an enum or not.

You can detect this: choice is Enum. Perhaps it would make the code a bit more robust if that test is used?

ndrslmpk commented 3 weeks ago

Any Updates on this?

mosuem commented 3 weeks ago

With the replacement of package:intl being worked on, package:intl4x, we are currently not planning to implement new features here. Happy to accept PRs though!