Skip to content

Clean up hard coded literals. #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+37 −53
Open

Clean up hard coded literals. #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0265756
-
polina-c Oct 10, 2025
81dd5b3
Merge branch 'main' of https://github.com/flutter/genui into tmp
polina-c Oct 10, 2025
caa1f02
Update simple_items.dart
polina-c Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 29 additions & 53 deletions packages/flutter_genui/lib/src/catalog/core_widgets/column.dart
View file
Open in desktop
Copy link
Collaborator

@jacobsimionato jacobsimionato Oct 10, 2025

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -9,32 +9,27 @@ import 'package:json_schema_builder/json_schema_builder.dart';
import '../../model/catalog_item.dart';
import '../../primitives/simple_items.dart';

enum _P { mainAxisAlignment, crossAxisAlignment, children, spacing }
Copy link
Collaborator

@jacobsimionato jacobsimionato Oct 10, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@gspencergoog gspencergoog Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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');

Copy link
Collaborator

@jacobsimionato jacobsimionato Oct 19, 2025

Choose a reason for hiding this comment

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

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(),


final _schema = S.object(
properties: {
'mainAxisAlignment': S.string(
_P.mainAxisAlignment.name: S.string(
description:
'How children are aligned on the main axis. '
'See Flutter\'s MainAxisAlignment for values.',
enumValues: [
'start',
'center',
'end',
'spaceBetween',
'spaceAround',
'spaceEvenly',
],
enumValues: MainAxisAlignment.values.map((e) => e.name).toList(),
),
'crossAxisAlignment': S.string(
_P.crossAxisAlignment.name: S.string(
description:
'How children are aligned on the cross axis. '
'See Flutter\'s CrossAxisAlignment for values.',
enumValues: ['start', 'center', 'end', 'stretch', 'baseline'],
enumValues: CrossAxisAlignment.values.map((e) => e.name).toList(),
),
'children': S.list(
_P.children.name: S.list(
items: S.string(),
description: 'A list of widget IDs for the children.',
),
'spacing': S.number(
_P.spacing.name: S.number(
description: 'The spacing between children. Defaults to 8.0.',
),
},
Expand All @@ -47,52 +42,33 @@ extension type _ColumnData.fromMap(JsonMap _json) {
String? mainAxisAlignment,
String? crossAxisAlignment,
}) => _ColumnData.fromMap({
'children': children,
'spacing': spacing,
'mainAxisAlignment': mainAxisAlignment,
'crossAxisAlignment': crossAxisAlignment,
_P.children.name: children,
_P.spacing.name: spacing,
_P.mainAxisAlignment.name: mainAxisAlignment,
_P.crossAxisAlignment.name: crossAxisAlignment,
});

List<String> get children =>
((_json['children'] as List?) ?? []).cast<String>();
double get spacing => (_json['spacing'] as num?)?.toDouble() ?? 8.0;
String? get mainAxisAlignment => _json['mainAxisAlignment'] as String?;
String? get crossAxisAlignment => _json['crossAxisAlignment'] as String?;
((_json[_P.children.name] as List?) ?? []).cast<String>();
double get spacing => (_json[_P.spacing.name] as num?)?.toDouble() ?? 8.0;
String? get mainAxisAlignment => _json[_P.mainAxisAlignment.name] as String?;
String? get crossAxisAlignment =>
_json[_P.crossAxisAlignment.name] as String?;
}

MainAxisAlignment _parseMainAxisAlignment(String? alignment) {
switch (alignment) {
case 'start':
return MainAxisAlignment.start;
case 'center':
return MainAxisAlignment.center;
case 'end':
return MainAxisAlignment.end;
case 'spaceBetween':
return MainAxisAlignment.spaceBetween;
case 'spaceAround':
return MainAxisAlignment.spaceAround;
case 'spaceEvenly':
return MainAxisAlignment.spaceEvenly;
default:
return MainAxisAlignment.start;
}
}
MainAxisAlignment _parseMainAxisAlignment(String? alignment) =>
parseEnum<MainAxisAlignment>(
alignment,
MainAxisAlignment.values,
MainAxisAlignment.start,
);

CrossAxisAlignment _parseCrossAxisAlignment(String? alignment) {
switch (alignment) {
case 'start':
return CrossAxisAlignment.start;
case 'center':
return CrossAxisAlignment.center;
case 'end':
return CrossAxisAlignment.end;
case 'stretch':
return CrossAxisAlignment.stretch;
default:
return CrossAxisAlignment.start;
}
}
CrossAxisAlignment _parseCrossAxisAlignment(String? alignment) =>
parseEnum<CrossAxisAlignment>(
alignment,
CrossAxisAlignment.values,
CrossAxisAlignment.start,
);

final column = CatalogItem(
name: 'Column',
Expand Down
8 changes: 8 additions & 0 deletions packages/flutter_genui/lib/src/primitives/simple_items.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,11 @@
// found in the LICENSE file.

typedef JsonMap = Map<String, Object?>;

T parseEnum<T>(String? value, List<T> values, T defaultValue) {
Copy link
Collaborator

@jacobsimionato jacobsimionato Oct 10, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@gspencergoog gspencergoog Oct 10, 2025

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

@jacobsimionato jacobsimionato Oct 19, 2025

Choose a reason for hiding this comment

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

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

if (value == null) return defaultValue;
return values.firstWhere(
(T e) => (e as Enum).name == value,
orElse: () => defaultValue,
);
}
Loading