Clean up hard coded literals.

[polina-c]

Contributes to #358

If looks good, will ask AI to update other catalog items the same way.

[gemini-code-assist]

Code Review

This pull request does a great job of cleaning up hard-coded literals by introducing an enum for property names and dynamically generating enum values for the schema. The refactoring of enum parsing logic into a reusable generic function is also a significant improvement.

I have a couple of suggestions to further improve the code:

1. In simple_items.dart, the new parseEnum function can be made more efficient and idiomatic by using .name on enums instead of toString().split().
2. In column.dart, the hard-coded default spacing value 8.0 can be extracted into a constant to eliminate the magic number, which aligns with the goal of this PR.

[jacobsimionato]

This is great!

Nit: Can we have named params just so it's a bit more obvious what the parameters mean at the call sites?

[gspencergoog]

I agree with Jacob about the named parameters.

Also, it's too bad we can't eliminate the List<T> values and just use T.values in the implementation. (I know we can't, it's just too bad).

[jacobsimionato]

Thanks for iterating. I'd still like to see the named parameters here before submitting this!

[jacobsimionato]

I think maybe constant strings are a better fit here than an enum, seeing as the constants here aren't really interchangeable instances of something, i.e. there are no specific polymorphism requirements.

E.g. maybe mainAxisAlignmentPropertyName and crossAxisAlignmentPropertyName.

[gspencergoog]

So, how would that look? Something like this?:

abstract base class FauxEnum {
	String get name;
	List<String> get values;
}

final class _P extends FauxEnum {
  const _P(this.name);
  String name;
  static const mainAxisAlignment = _P('mainAxisAlignment');
  static const crossAxisAlignment = _P('crossAxisAlignment');
  static const children = _P('children');
  static const spacing = _P('spacing');
  List<String> get values => [
    mainAxisAlignment,
    crossAxisAlignment,
    children,
    spacing,
  ].map<String>((v) => v.name).toList();
}

And I guess you'd have to make a base class for the class FauxEnum with the static strings that has the value accessor in order for the parseEnum function to work similarly. But maybe you could eliminate the values arg to parseEnum then, since values wouldn't have to be static?

This is a nice cleanup, but one issue I have with all of this (including the enums) is that if there is a property that ends up being a Dart keyword, then we could have issues. (e.g. static const String class = _P('class'); wouldn't work). The current implementation doesn't have that problem.

At least with the constant strings, you could still implement it, it's just that the name would be different from the value: static const String clazz = _P('class');

[jacobsimionato]

Oh I propose that we keep using the enums for MainAxisAlignment, CrossAxisAlignment here. These are from Flutter and they make sense as enums, and the parseEnum function is neat and works well in my opinion. These enums aren't used to define the string "MainAxisAlignment", but rather the different alignments that are available.

For the property names e.g. "mainAxisAlignment", "crossAxisAlignment", I vote we instead just have something like:

const mainAxisAlignmentPropertyName = `mainAxisAlignment`;

final _schema = S.object(
  properties: {
    mainAxisAlignmentPropertyName: S.string(
      description:
          'How children are aligned on the main axis. '
          'See Flutter\'s MainAxisAlignment for values.',
      enumValues: MainAxisAlignment.values.map((e) => e.name).toList(),

[jacobsimionato]

I'd love to see what @gspencergoog things too before we go and change the entire codebase. I think I'm strongly in favor of the parseEnum utility, and mildly in favor of using the constants.

[polina-c]

Thanks for the comments. I will respond and then I will make sure we all have agreement here before merging (it is good that it is not urgent). So, it will wait for Jacob to be back from OOO.