google / built_value.dart

Immutable value types, enum classes, and serialization.
https://pub.dev/packages/built_value
BSD 3-Clause "New" or "Revised" License
870 stars 184 forks source link

Support reverse lookup of wireNames #663

Open larssn opened 5 years ago

larssn commented 5 years ago

A small feature request to have wireName values be part of the reverse lookup that valueOf provides.

If I have an enum like this:

class CountryCode extends EnumClass {
  @BuiltValueEnumConst(wireName: 'DK')
  static const CountryCode DENMARK = _$dk;

  @BuiltValueEnumConst(wireName: 'CH')
  static const CountryCode SWITZERLAND = _$ch;

  const CountryCode._(String name) : super(name);

  static Serializer<CountryCode> get serializer => _$countryCodeSerializer;
  static BuiltSet<CountryCode> get values => _$values;
  static CountryCode valueOf(String name) => _$valueOf(name);
}

the generated valueOf function looks like this:

CountryCode _$valueOf(String name) {
  switch (name) {
    case 'DENMARK':
      return _$dk;
    case 'SWITZERLAND':
      return _$ch;
    default:
      throw new ArgumentError(name);
  }
}

But there's no way to do CountryCode.valueOf('CH'), even though 'CH' might be the only value available in the program flow. So the optimal generated valueOf would include this:

CountryCode _$valueOf(String name) {
  switch (name) {
    case 'DENMARK':
      return _$dk;
    case 'SWITZERLAND':
      return _$ch;
    case 'DK':
      return _$dk;
    case 'CH':
      return _$ch;
    default:
      throw new ArgumentError(name);
  }
}
davidmorgan commented 5 years ago

Sorry I missed this!

It definitely seems like this functionality should be provided somehow.

I'm a little worried about what happens when wirename and name clash. For example:

  @BuiltValueEnumConst(wireName: 'SWITZERLAND')
  static const CountryCode DENMARK = _$dk;

  @BuiltValueEnumConst(wireName: 'DENMARK')
  static const CountryCode SWITZERLAND = _$ch;

...now the switch statement can't do anything sensible, and if you're expecting it to work, you'll be surprised.

How about giving you the option to make both name and valueOf use the wireName?

@BuiltValueEnum(useWireName: true)
class CountryCode extends EnumClass {
  @BuiltValueEnumConst(wireName: 'DK')
  static const CountryCode DENMARK = _$dk;

  @BuiltValueEnumConst(wireName: 'CH')
  static const CountryCode SWITZERLAND = _$ch;

  const CountryCode._(String name) : super(name);

  static Serializer<CountryCode> get serializer => _$countryCodeSerializer;
  static BuiltSet<CountryCode> get values => _$values;
  static CountryCode valueOf(String name) => _$valueOf(name);
}
larssn commented 5 years ago

Your suggestion sounds reasonable enough, and you're right, the first example would mess up the switch statement.

davidmorgan commented 5 years ago

SG, I'll try to take a look next time I do a batch of features :)

davidmorgan commented 4 years ago

Thinking about this a bit more, I'm not 100% sold on useWireName: true. It feels like we're bound to hit a case where someone wants both.

How about an optional static method you can write called e.g. fromWireName? I can't think of any nicer way to add the functionality, open to suggestions...

larssn commented 4 years ago

Isn't that the same as implementing a manual lookup method? I was hoping for something simple (that isnt boilerplate heavy), that happens automatically.

I still think your original suggestion is good. I cant think of a real life scenario where you would have SWITZERLAND = DENMARK. You could just make it throw if it hits that scenario, like it does when null data is passed into a non nullable field.

davidmorgan commented 4 years ago

You would declare fromWireName, but you would not have to write it :) i.e. something like

static Foo fromWireName(String name) => _$fromWireName;

and _$fromWireName would be generated.

kuhnroyal commented 4 years ago

Is there any progress on this, also for wireNumber?

davidmorgan commented 4 years ago

No progress, sorry; it's likely to be late Q4 / early Q1 before there is another batch of built_value improvements, as I'm going to wait for null safety to be released in the SDK first.

kuhnroyal commented 4 years ago

Thanks, that makes sense.